JDK-4294756 : Javac should warn if synchronized method is overridden with a non synchronized
  • Type: Enhancement
  • Component: tools
  • Sub-Component: javac
  • Affected Version: 1.2.2
  • Priority: P4
  • Status: Closed
  • Resolution: Won't Fix
  • OS: generic
  • CPU: generic
  • Submitted: 1999-11-25
  • Updated: 1999-12-21
  • Resolved: 1999-12-21

Name: skT88420			Date: 11/25/99

java version "1.2"
Classic VM (build JDK-1.2-V, green threads, sunwjit)

According to the Java Language Specification (or more precisely it does not say
anything about this (or I have not found it)) it is OK to override a
synchronized method with a non synchronized one. If this is good or bad might be
discussed, but to exemplify why I think this is bad I'll give you an example.

First we have 3 classes, Thread1, Thread2 and Bar. Both Thread1 and Thread2 have
references to the same Bar instance and they execute the same method repeatedly
in Bar, synchronizedMethod, I.e. Bar looks like this

public class Bar {
  public synchronized void synchronizedMethod() {
     for (int i=0; i<10; i++) System.out.print(i);

This works OK, since synchronizedMethod() is synchronized, and both Thread1 and
Thread2 assume that whenever they use an instance of a Bar method the
synchronizedMethod() is synchronized.

Then we add a 4th class, a subclass to Bar called Foo

public class Foo extends Bar {
   public void synchronizedMethod() {
     for (int i=0; i<10; i++) System.out.print(i);

Now we will have problems if a Foo instance is casted to a Bar instance and this
instance is given to both Thread1 and Thread2. Since Thread1 and Thread2 can not
tell the difference between a Bar and a Foo (since they wrere written before Foo
was ever thought of), they will gladly assume that the Bar instance they are
given is properly synchronized...

Declaring a method as synchronized is like signing a contract, promising that
the method will always be sysnchronized. And if this contract is ever broken,
unexpected/gruelsome consequencies might occur.

One more thing. There is a subtle difference between

public void synchronized foo() { } // 1


public void foo() { synchronized(this) { } } // 2

The difference is that in the first case the compiler will mark the synchronized
method with a flag and in the other case it will insert monitorenter/monitorexit
bytecodes. I am only worried about the first case.

This subtle difference makes it very easy for the compiler to issue a warning
whenever the first case is found, and I think that would be *very* good to have
that waring. I don't think you can change the spec without messing up a lot of
code (for instance by enforcing all synchronized methods to stay synchronized
when they are overridden), right?
(Review ID: 98287) 

WORK AROUND Name: skT88420 Date: 11/25/99 This can only be adressed with coding standards, but a coding standard can be broken very easily, either consciuosly or not. And if the coding standard is broken, it might trigger very hard-to-find bugs which in turn becomes very costly... ======================================================================

EVALUATION Whether a method is synchronized or not is *not* considered a part of its signature in Java. That is, it is not part of that portion of the contract of the method that language requires the compiler to check automatically. The use of the synchronized modifier, as well as other synchronization via explicit 'synchronized' statements, is a part of the implementation of an abstraction represented by a class, and an alternate implementation captured in a subclass may use a different synchronization strategy in order to implement equivalent semantics. As an example, consider the case in which a small critical section (protected by a 'synchronized' statement) within a larger unsynchronized method replaces a smaller method that was protected in its entirety by a synchronized method modifier. From the standpoint of the user of a class, the relevant contract is whether the methods of the class can be safely invoked by multiple concurrent threads, or what constraints on concurrent access must be observed. This is in general more subtle than what can be captured by the presence or absence of a synchronized method modifier, and should be documented by other means. A lint-like tool could be written to warn of possible problems in cases such as the submitter describes, but such an unreliable heuristic does not belong in the compiler. We would generally not consider putting warning messages into the compiler except for usages that have been formally deprecated in the language specification. william.maddox@Eng 1999-12-20