Community
Participate
Working Groups
See https://bugs.eclipse.org/bugs/show_bug.cgi?id=249343 for some discussion. We have found this warning to be mostly noise. Proxied or wrapped 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. Introducing deadlock. For example, the ClassLoader.loadClass(String, boolean) method is synchronized. Synchronizing on this method will introduce deadlock in OSGi classloaders, which can have cycles. Other comments from bug 249343: 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. I would like other teams to go through the same exercise to see how many of the warnings are noise vs. real issues. My vote is to disable this by default and let teams decide to enable it on a case by case basis.
There was one warning in core.resources, and it was invalid. It was an empty method so synchronization was not needed.
There was one warning in JDT APT, on a method that did a little extra unsynchronized work and then called the superclass. Adding synchronization was unnecessary but didn't hurt anything. I agree with the assertion that the synchronized keyword is not a public contract. Its absence does not mean that a method is not threadsafe; its presence does not mean that the method is threadsafe or deadlock safe. JSR-305 proposes annotations which do describe synchronization contracts.
Created attachment 114159 [details] Proposed patch
Released for 3.5M3. Fixed
Verified for 3.5M3 using I20081026-2000 build.
