Isn't @Synchronized feature useless and buggy?

[Victor Cordis]

Lombak is, in my opinion, a very interesting project which helps
productivity ... a Java beautifier and, as I saw on the issue tracker,
has support for all major Java IDEs (NetBeans, Eclipse, InteliJ).
But the question is which of it's features are really helpful:

I come to question the usefulness and correctness of the @Synchronized
feature:

In short: this feature does not really reduce the amount of Java code
one has to write:

1) "@Synchronized" is actually longer and with uppercase+lowercase
letters vs "syn" + autocomplete
2) the keyword shows up in the method definition and one knows which
monitor will be acquired
3) zero-length arrays as Objects with Monitors are indeed the best
choice for Serializable locks, but
do we really need to serialize a lock? No, one needs to mark the
locks as transient and create a
custom de-serialization method which will init the locks upon
deserialization.

Hence, I mark the @Synchronized annotation feature as not only
unhelpful (manual coding does the trick
just as fast) but also a serialization overhead

[Maaartin G]

On Tuesday, June 28, 2011 4:26:25 PM UTC+2, Victor Cordis wrote:

 1) "@Synchronized" is actually longer and with uppercase+lowercase
letters vs "syn" + autocomplete

I quite disagree: In order to get the equivalent behavior you need to define a lock and for each method name the lock and create a block. So it gets surely longer and less readable, especially because of the additional indentation.

The speed of typing of fixed expression is completely irrelevant for me: If I needed it a lot, I could define a template in Eclipse and save me all the typing. The readability is what matter, and here Lombok wins.

 

2) the keyword shows up in the method definition and one knows which
monitor will be acquired

I don't get it what you mean. Should this be a problem? Would you prefer it the lock required to be secret?

 

3) zero-length arrays as Objects with Monitors are indeed the best
choice for Serializable locks, but
    do we really need to serialize a lock? No, one needs to mark the
locks as transient and create a
    custom de-serialization method which will init the locks upon
deserialization. 

I think you're right, we don't need to serialize it. But what do we gain by doing something else? Actually, we save a couple of bytes, is this your reason for proposing it?

 

Hence, I mark the @Synchronized annotation feature as not only
unhelpful (manual coding does the trick
just as fast) but also a serialization overhead

Manual coding is simple, but make the code less clear. Using transient may be improve Lombok a bit.

[Fabrizio Giudici]

On 06/28/2011 07:11 PM, Maaartin G wrote:
>
>
> I think you're right, we don't need to serialize it. But what do we
> gain by doing something else? Actually, we save a couple of bytes, is
> this your reason for proposing it?
>

I agree with Maartin for all the points, but the serialization thing. I
didn't think of it before reading Victor's post, but I'd say that by
adding/removing a @Serializable to a method you're breaking backward
compatibility with existing serialized data, right? I'd say this is a
bug and the monitor should be really transient.

--
Fabrizio Giudici - Java Architect, Project Manager
Tidalwave s.a.s. - "We make Java work. Everywhere."
java.net/blog/fabriziogiudici - www.tidalwave.it/people
Fabrizio...@tidalwave.it

[Reinier Zwitserloot]

We can't really overwrite serialization, and if we don't, the lock's going to end up being null post-deserialize if we make the field 'transient' (right?). We can't lazily init it because then we'd need a lock to do _that_. We could add the readResolve magic method but what if you already have it, or later decide to add one? We'd then have to try and wiggle a 'lock = new Object();' in there someplace.

You're right, removing the last, or adding the first @Synchronized method will break your serialization protocols.

NB: Maaartin, if you use 3 @Synchronized in one class, you get just the one lock. It's a different lock from 'this' which was the point.

Separate from all that, @Synchronized is indeed buggy. Robbert Jan has tracked this down to the @SuppressWarnings that we added to the field. We can probably fix it for 0.10.1.

[Victor Cordis]

1) you're right on this one :-)
2) the "sync..." keyword appears in method def which comes from just
the .jar (bright-side)
in order to see the @Sync... one would need the javadoc (downside)
3) thinking like this would mean that even primitives like "byte" ,
"short", "boolean"
are of no use, we could use "int" instead... this is not
acceptable :D

Conclusion:
I just question the usefulness of @Sync ( I personally don't need
it)
but if I would, I would like it to generate some improved lock
implementation.

Thanks for the response

Victor

[Victor Cordis]

The way Java Serialization Overwriting is done is a bit ugly
(but there was no other way because of single inheritance (which is a
good thing) )
but can be an easy task, as specially in our lock case:

1) serialization:
simply mark the locks as transient and leaf it up to default
serialization
2) de-serialization:
simply define the private de-serialization method which would do
something like this:

private void readObject(java.io.ObjectInputStream s)
throws IOException, ClassNotFoundException {
// read the def serialization:
s.defaultReadObject();
// init the transient locks (which were not serialized)
this.mySafeLock = new Object();
}

The only down-side would be that the locks cannot be final anymore
( because the above method is not a constructor :-( )

So the code could be more efficient ( time & mem) but you do not
have a "final" guarantee...

Victor

[Victor Cordis]

On second thought:

since the entire point of @Sync is to generate lock attributes and
sync methods using the locks
=>
locks are not visible in the user code
=>
no risk of reiniting them with a new Object()
=>
I suggest that the @Sync implementation should do the following:

1) based on the @Sync(lockName) generate the lock attributes as
follows:
private transient Object nameLock= new Object()

2) generate the sync blocks

3) override de-serialization as follows:

private void readObject(java.io.ObjectInputStream s)
throws IOException, ClassNotFoundException {
// read the def serialization:
s.defaultReadObject();

//for each lock do
this.lockName= new Object();
}

=> missing final shouldn't be a problem + efficient serialization
=> BUT: problem when the user also wants to define a custom de-
serialization => compiler error on the generated code
=> one would need to check if the class contains the de-ser override
and, if yes then append the lock inits to it

[Reinier Zwitserloot]

"simply"? What if the class already contains a readObject method? The cost of non-final on the lock is also significant, in my opinion. Certainly more significant than "If you remove all @Synchronized, you break serialization of that class".

The reason the locks aren't public is because our philosophy is that they shouldn't be public. Locking is an implementation detail. A synchronized method hints that it might be thread safe in some fashion or another.  The docs will need to explain in which ways it is thread safe, because it's not a black and white issue. A method that does not have 'synchronized' in its signature can still be thread safe, so either way you have to look at the docs for the details. What good is synchronized, then?

As far as I know, a subclass can override a method and simply not add synchronized, so its significance is virtually zero. Hence the idea: That should never have been public information in the first place.

synchronized, the method modifier keyword, also uses 'this' (or Self.class for static methods) as lock object. So, a public lock then. public locks lead to deadlocks.

 --Reinier Zwitserloot

--
You received this message because you are subscribed to the Google
Groups group for http://projectlombok.org/

To post to this group, send email to project...@googlegroups.com
To unsubscribe from this group, send email to
project-lombo...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/project-lombok?hl=en

[Roel Spilker]

Also, someone mentioned that removing the last @Synchronized method will break deserialization. That would only be the case if no serialVersionUID was provided and the object is send to a different JVM containing a different version or stored on disk. If you're going to store it on disk, you better provide a serialVersionUID or use a serialization proxy object.

If you set a serialVersionUID and you deserialize an object that in a previous version has an extra field, AFAIK that field will just not be set. So no problems there.

Roel

On 30-6-2011 15:53, Reinier Zwitserloot wrote:

"simply"? What if the class already contains a readObject method? The cost of non-final on the lock is also significant, in my opinion. Certainly more significant than "If you remove all @Synchronized, you break serialization of that class".

The reason the locks aren't public is because our philosophy is that they shouldn't be public. Locking is an implementation detail. A synchronized method hints that it might be thread safe in some fashion or another. �The docs will need to explain in which ways it is thread safe, because it's not a black and white issue. A method that does not have 'synchronized' in its signature can still be thread safe, so either way you have to look at the docs for the details. What good is synchronized, then?

As far as I know, a subclass can override a method and simply not add synchronized, so its significance is virtually zero. Hence the idea: That should never have been public information in the first place.

synchronized, the method modifier keyword, also uses 'this' (or Self.class for static methods) as lock object. So, a public lock then. public locks lead to deadlocks.

�--Reinier Zwitserloot

On Wed, Jun 29, 2011 at 5:44 PM, Victor Cordis <cordis...@gmail.com> wrote:

The way Java Serialization Overwriting is done is a bit ugly
(but there was no other way because of single inheritance (which is a
good thing) )
but can be an easy task, as specially in our lock case:

1) serialization:

� �simply mark the locks as transient and leaf it up to default
serialization
2) de-serialization:
� �simply define the private de-serialization method which would do
something like this:

� �private void readObject(java.io.ObjectInputStream s)
� � � � � �throws IOException, ClassNotFoundException {
� � � �// read the def serialization:
� � � �s.defaultReadObject();
� � � �// init the transient locks (which were not serialized)
� � � �this.mySafeLock = new Object();
� �}

� �The only down-side would be that the locks cannot be final anymore
� �( because the above method is not a constructor :-( � )

� �So the code could be more efficient ( time & mem) but you do not
� �have a "final" guarantee...

�

[Reinier Zwitserloot]

I'll drive the stake through the heart here, fellas:

If you serialize and deserialize an object with a transient field in it, that field ends up being null. Thus, we'd have to de-final the lock field, and we'd need to add a readObject method.

If you use serialVersionUID, and you serialize with some @Synchronized annotations in the class, then remove those annotations, recompile your code, and deserialize with this new classbase, everything is just peachy. That stored empty object array is simply discarded as there's no longer a field to put it in.

I'm glad Victor and Fabrizio brought this up. I've added a note about this to the fineprint of the Synchronized.html feature docs (I just published those to projectlombok.org, so if you want to have a look: http://projectlombok.org/features/Synchronized.html ). And now that we've determined there is virtually zero downside to how we do things, serialization-wise, I'm axing this feature request.

[Roel Spilker]

Actually, we could consider making the $lock field non-final and
volatile, and use a getter to fill the field if it is null. That way
deserialization would still work if you add a Synchronized annotation
after some objects are already serialized. The only problem I see is
that we would need to make sure that the getter works thread-safe, and I
don't see how we can do that without locking on 'this'.

[Victor Cordis]

Sync keyword is OK because:

1) Locks and data are procedural ( c-style) way of thinking. OOP means
each Object is a resource
with it's own monitor (a.k.a. lock) and they can be acquired and
released between threads (using synchronized).
=> creating Object locks when you only need 1 is redundant


2) Thread-safe is a gray issue only if you need more than one lock.
For 1 lock sync (like java.util.Hashtable)
it's pretty black and white... so sync in method definition means: no
javadocs needed + syntax sugar

> As far as I know, a subclass can override a method and simply not add
> synchronized, so its significance is virtually zero.

Override means same footprint ( name + paramList) => access or
synchronized can be changed within certain limits.

> Hence the idea: That
> should never have been public information in the first place.

Wrong: sync modifier can be excluded because the new implementation
may not need to be synchronized anymore
(so the syntax sugar does not have unforeseen consequences )
I think the guys at Sun really gave it a though and got this right !

3) It is vital to know IF and WHEN you acquire any resource
Using a class with 1 private lock instead of "this" can be
deadlock prone as well,
you just load the JVM with one unused object.


@Sync annotation is NOT OK because:

A) it is just a syntax sugar no better than "sync in method
definition"

B) only need to serialize data... NEVER locks
( it's like trying to serialize an ArrayList together with a Stateless
session bean.... what's the point ? )

=> it attempts to solve a non-existing issue and brings a
serialization issue


BTW: what's the big deal in loosing the "final" keyword of the lock
attribute if you
don't even get to see the attribute in your source code? You cannot
mess up what
you can't see.

[Reinier Zwitserloot]

Your plight has not moved me. We won't change @Synchronized. (Well, other than fix the 'can't find class' bug).

 --Reinier Zwitserloot

--

[Maaartin G]

On Wednesday, June 29, 2011 9:47:37 AM UTC+2, Reinier Zwitserloot wrote:

NB: Maaartin, if you use 3 @Synchronized in one class, you get just the one lock. It's a different lock from 'this' which was the point.

Of course they may be only one lock. I must have expressed myself in a quite confusing way.

On Thursday, June 30, 2011 5:49:50 PM UTC+2, Victor Cordis wrote:

BTW: what's the big deal in loosing the "final" keyword of the lock 
attribute if you
don't even get to see the attribute in your source code? You cannot
mess up what
you can't see.

I'm not sure, but what if one thread passes a newly created object to another one? There's no guarantee the assignment can be seen  by the second thread, is there? So you could get an NPE because of synchronizing on null.

[Reinier Zwitserloot]

You can see them and even use them; $lock becomes a legal field, not much we can do about that. In theory we could hunt down all usage and mark it as warning/error, I guess. We don't do that right now at any rate. In fact, you might want to explicitly synchronize on $lock elsewhere.

 --Reinier Zwitserloot