Community
Participate
Working Groups
See http://download.eclipse.org/eclipse/downloads/drops/I20080930-0921/compilelogs/plugins/org.eclipse.osgi_3.5.0.v20080929-1800/@dot.bin.html We compile against foundation during the build. It appears the foundation library does not have as many synchronous methods. When I compile org.eclipse.osgi against java5 I get 16 of these these warnings instead of the 5 that show up in the build.
I released fixes for 4 out the 5 warnings from the build along with a few others that popped up when compiling against Java5. There are three cases that I am concerned about adding synchronized methods for. 1) Cases where we proxy objects. In this case synchronizing on (this) at the method level which simply turns around and calls the same method on the proxied object seems like a bad practice. We would add another level of locks for no reason. 2) The ClassLoader.loadClass(String, boolean) method. Synchronizing on this method will introduce deadlock in OSGi classloaders, which can have cycles. 3) The classes from the OSGi alliance. We need to go back to OSGi to justify adding the synchrounous keywords where needed. The build currently does not show warnings in OSGi code because the foundation library we are compiling against must not have the same synchronous methods as Java5. Leaving this bug open for more discussion and possibly disabling the warnings for the org.eclipse.osgi project
(In reply to comment #1) > > 2) The ClassLoader.loadClass(String, boolean) method. Synchronizing on this > method will introduce deadlock in OSGi classloaders, which can have cycles. This warning is not showing up in the build because of how we compile org.eclipse.osgi against an exceptions.jar + Foundation 1.0. The exceptions.jar has a few extra classes in it for us to compile against which we use at runtime if the VM has them (i.e. java.nio). This jar includes a stubbed ClassLoader class that has the definePackage method (not available on minimum). The stubbed ClassLoader class does not contain any synchronous methods. This is why we don't see the classloader.loadClass warnings in the build. > > 3) The classes from the OSGi alliance. We need to go back to OSGi to justify > adding the synchrounous keywords where needed. The build currently does not > show warnings in OSGi code because the foundation library we are compiling > against must not have the same synchronous methods as Java5. Warnings in OSGi classes are from Exception classes that override Throwable.initCause. This is a synchronous method added in Java 1.4. We compile against Foundation 1.0 which does not include this method. To the compiler the method looks like a new method and it has no idea that the method is exists and is synchronized in 1.4 or higher.
This seems like a very silly warning (but I am sure it has been over-discussed somewhere else). Synchronization is an implementation detail of the method. It is not a public contract like the method signature. I could easily remove the synchronized keyword from the method declaration and wrap then entire method body in synchronized (this) {...} with the same result. This is why none of the EEs from OSGi have any synchronized keywords. It is implementation detail. We should disable this warning/error in our compiler preferences.
(In reply to comment #3) > This seems like a very silly warning (but I am sure it has been over-discussed > somewhere else). > > Synchronization is an implementation detail of the method. It is not a public > contract like the method signature. I could easily remove the synchronized > keyword from the method declaration and wrap then entire method body in > synchronized (this) {...} with the same result. > > This is why none of the EEs from OSGi have any synchronized keywords. It is > implementation detail. > > We should disable this warning/error in our compiler preferences. > I agree, in most cases I don't think this warning points to a real problem. We can disable the warning/error for our project preferences but we also need to disable the warning in the build (which does not use our project preferences).
Warning levels in the build can be set on individual bundles using the javacWarnings.<library> property in your build.properties file. So to turn off a specific warning: javacWarnings..=-<option> However, looking at the JDT docs for the warnings http://help.eclipse.org/ganymede/index.jsp?topic=/org.eclipse.jdt.doc.isv/guide/jdt_api_compile.htm I don't see any that have anything to do with synchronization.
The warning option is: syncOverride It is not documented as on by default. So this should also be clarified.
Note that this warning did not exist in Ganymede, hence the Ganymede doc doesn't reflect it. (Note that the compiler printUsage message did not indicate warning being on either, fixed in HEAD).
I can't believe that this warning is on by default. That seems very wrong given synchronized is an implementation detail. Tom, some of the changes you checked in to avoid the warning are just putting synchronized on an empty method body which is a pointless change. They should be backed out. We need to turn off this warning for the equinox projects. Or campaign to have the default changed such that the warning is off by default.
I'll send a note to eclipse-dev to start the conversation. My findings have not surfaced any real issues where the warning makes sense. I would like to know if other teams have found real bugs.
BJ - yes it is an impl detail, but with drastic consequences when unintentional. The reason we enabled it by default is to find out if this is a real scenario or just a mistake. You seem to have a good case, and thus I am willing to disable it by default now. Note that it found couple of real issues elsewhere. Tom - would you mind entering a bug against JDT/Core about the default ?
Done. See bug 249535
The compiler default has changed to ignore. I backed out the unnecessary code changes I made earlier.
