Bug 249535

Summary:

[compiler] Consider disabling the syncOverride warning/error by default

Product:

[Eclipse Project] JDT

Reporter:

Thomas Watson <tjwatson>

Component:

Core

Assignee:

Philipe Mulet <philippe_mulet>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

eclipse, hargrave, john.arthorne, markus.kell.r

Version:

3.4

Target Milestone:

3.5 M3

Hardware:

OS:

Windows XP

Whiteboard:

Attachments:

Description	Flags
Proposed patch	none

Description Thomas Watson

2008-10-02 15:18:21 EDT

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.

Comment 1 John Arthorne

2008-10-02 16:17:23 EDT

There was one warning in core.resources, and it was invalid. It was an empty method so synchronization was not needed.

Comment 2 Walter Harley

2008-10-02 18:43:24 EDT

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.

Comment 3 Philipe Mulet

2008-10-03 03:51:10 EDT

Created attachment 114159 [details]
Proposed patch

Comment 4 Philipe Mulet

2008-10-06 11:50:46 EDT

Released for 3.5M3.
Fixed

Comment 5 Kent Johnson

2008-10-28 13:11:24 EDT

Verified for 3.5M3 using I20081026-2000 build.