Re: Review request for 6927486: Deadlock in legacy Hashtable writeObject()

[Mike Duigou]

Everything looks resolved to me as well. I will push this webrev over the 
weekend or on Monday. Thank you for your patience in working through the 
pedantic details. Hopefully this will make the process smoother for all future 
commits and we won't have to revisit any of these issues.

Mike

On Feb 11 2011, at 04:05 , Alan Bateman wrote:

> Neil Richards wrote:
>> Hi Alan, Mike,
>> Please find attached a further webrev zip file with the IBM copyright
>> split out into its own little comment block, as requested.
>>  
> Good to see this finally resolved. Thumbs up from me. Mike has offered to 
> push this so I'll leave it to him.  Also I assume this means the related fix 
> to j.u.Vector can be brought to the finish line too, right?
> 
> -Alan.

Re: Review request for 6927486: Deadlock in legacy Hashtable writeObject()

[Alan Bateman]

Neil Richards wrote:

Hi Alan, Mike,
Please find attached a further webrev zip file with the IBM copyright
split out into its own little comment block, as requested.
  
Good to see this finally resolved. Thumbs up from me. Mike has offered 
to push this so I'll leave it to him.  Also I assume this means the 
related fix to j.u.Vector can be brought to the finish line too, right?


-Alan.

Re: Review request for 6927486: Deadlock in legacy Hashtable writeObject()

[Alan Bateman]

Neil Richards wrote:

Hi Alan, Mike,
Please find attached one more webrev zip, with updated license text
and now based off jdk7-b128.

Let me know if this is now good to be committed, or if there's
anything else I need to do,
Thanks,
Neil
  
Thanks for perceiving with this. The only thing that looks a bit strange 
(to me anyway) is that the GPL header is in the same comment block as 
the IBM notice. I just checked other examples in the repository and the 
convention seems to be to put them in separate comment blocks.


-Alan.

Re: Review request for 6927486: Deadlock in legacy Hashtable writeObject()

[Alan Bateman]

Neil Richards wrote:

:
Okay. I've updated the changeset to do this too, and attached this as
a webrev zip file (as per Dalibor's suggestion).

Please review this modified changeset, and let me know if anything
further is required for its acceptance.
  
Looks fine to me.  As per the tests for the Vector serialization, you 
might want to check the format of the date range in the headers.


I hate bringing up code style/conventions (as it only starts wars) but 
you might want to check the "implements Serializable" at L76 of 
SerializationDeadlock.java as it looks like it's not indented. Same 
thing with the "throws IOException" as line 82. I can't tell from the 
webrev if this is due to tabs or something else but I noticed it in the 
webrev for the Vector fix too.

:
I did post an update which added the appropriate headers here:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2010-December/005529.html
  
I didn't see that one, sorry. Anyway, I think Mike is going help get 
these pushed.


-Alan

Re: Review request for 6927486: Deadlock in legacy Hashtable writeObject()

[Stuart Marks]

On 1/5/11 5:36 AM, Alan Bateman wrote:

I've taken the liberty to generate a webrev from the changeset, just to make it
a bit easier for folks to browse and review.
http://cr.openjdk.java.net/~alanb/6927486/webrev/

I don't see any issues with the changes to j.u.Hashtable. You might have seen
Stuart Mark's changes go by recently where he changed some of the existing code
(including java.util) to use diamond. You might want to use this in these
changes to avoid needing to re-run the tools on this code. This would also fix
a style issue where you've got a space between the type parameters in a few
places.


A couple things here.

First, the changeset Alan refers to is 449dfb061fde, where I used a conversion 
tool to apply the diamond operator in a bunch of places. The tool applied 
diamond *everywhere* that the current JDK 7 compiler supports it. It's not 
clear that we actually want to use diamond everywhere that it can be used, 
however, we haven't completely decided this yet.


In Hashtable.java, diamond is used in a bunch of similar places. I'd suggest 
following the style that's there and using diamond in this case, the creation 
of an Entry object for entryStack, e.g.


entryStack = new Entry<>(0, entry.key, entry.value, entryStack);

Second, it's not clear to me that there is a preferred style for putting spaces 
in the list of type arguments. If one looks around the code base, it's quite 
inconsistent. There does seem to be a moderate preference for not using spaces 
when the type arguments are themselves type parameters, which are typically 
single letters, e.g. Entry. But spaces are often used when the type 
arguments are concrete types that are full words, e.g. Map. 
But this is moot if diamond is used in this case.


s'marks

Re: Review request for 6927486: Deadlock in legacy Hashtable writeObject()

[Alan Bateman]

Neil Richards wrote:

Please find attached a changeset to address the problem reported in
bug 6927486, "Deadlock in legacy Hashtable writeObject()".
  
I've taken the liberty to generate a webrev from the changeset, just to 
make it a bit easier for folks to browse and review.

 http://cr.openjdk.java.net/~alanb/6927486/webrev/

I don't see any issues with the changes to j.u.Hashtable. You might have 
seen Stuart Mark's changes go by recently where he changed some of the 
existing code (including java.util) to use diamond. You might want to 
use this in these changes to avoid needing to re-run the tools on this 
code. This would also fix a style issue where you've got a space between 
the type parameters in a few places.


In the tests you catch Exception (or IOException or 
ClassNotFoundException) and then re-throw a RuntimeException. You can 
probably remove this as jtreg will mark the test as failed if it 
completes with any exception.


I think Mike Duigou plans to review this too and help get the changes 
into the tl/jdk repo.



The problem reported is similar to one found in java.util.Vector, for
which a fix is also currently under review
(http://mail.openjdk.java.net/pipermail/core-libs-dev/2010-December/005529.html).
  
You might want to give this one a nudge by re-sending with a GPL header 
on the test. As I recall it was just the test that needed to be reviewed.

PPS:
I notice the API javadoc for PropertyPermission does not give its
serialized form (and the "See Also:" section), even though it is
Serializable, which strikes me as unusual.

I suppose it has no serializable fields of its own (over that which
BasicPermission has), but I'm sure I've seen other serializable
classes with no fields having "serialized form" entries.

  
I assume it's the @serial exclude. You'll find some background in 
4288648 (from 11 years ago) but it would require further digging to find 
all the details.


-Alan

Re: Review request for 6927486: Deadlock in legacy Hashtable writeObject()

[Dalibor Topic]

On 12/22/10 12:19 PM, Neil Richards wrote:
> On 22 December 2010 09:38, David Holmes  wrote:
>> Hi Neil,
>>
>> Any chance you can generate webrevs as mentioned in:
>>
>> http://openjdk.java.net/guide/changePlanning.html
> 
> Hi David,
>>From the documentation, I don't believe I can do so yet.
> 

Hi Neil,

As a workaround, if you fetch the webrev script from 
http://blogs.sun.com/jcc/resource/webrev and run it on your hg tree, 
it will generate a webrev.zip file, along with a  webrev, which you can 
send to this list as an attachment until you get an account on cr.openjdk. 

For an introduction to webrev, please see 
http://blogs.sun.com/jcc/entry/webrev_for_openjdk_a_code

cheers,
dalibor topic

-- 
Oracle 
Dalibor Topic | Java F/OSS Ambassador
Phone: +494023646738  | | | Mobile: +491772664192 

Oracle Java Platform Group

ORACLE Deutschland B.V. & Co. KG | Nagelsweg 55 | 20097 Hamburg

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstr. 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V.
Rijnzathe 6, 3454PV De Meern, Niederlande
Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697
Geschäftsführer: Jürgen Kunz, Marcel van de Molen, Alexander van der Ven

Green Oracle  Oracle is committed to 
developing practices and products that help protect the environment

Re: Review request for 6927486: Deadlock in legacy Hashtable writeObject()

[Neil Richards]

On 22 December 2010 09:38, David Holmes  wrote:
> Hi Neil,
>
> Any chance you can generate webrevs as mentioned in:
>
> http://openjdk.java.net/guide/changePlanning.html

Hi David,
>From the documentation, I don't believe I can do so yet.

http://cr.openjdk.java.net/ (referred to from the page you mention) says:

"Any user with push access to the OpenJDK Mercurial server can publish
materials on this server."

As I don't think I have yet been granted push access to the OpenJDK
Mercurial server, I'm not in a position to publish webrevs for review.

I've tried to follow the documented process - as best as I can figure
out (!) - from http://openjdk.java.net/contribute/ and
http://openjdk.java.net/guide/producingChangeset.html

Please let me know if there's anything else (within my power) I can do
to aid the review process.

Cheers,
Neil

--
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Review request for 6927486: Deadlock in legacy Hashtable writeObject()

[Neil Richards]

Please find attached a changeset to address the problem reported in
bug 6927486, "Deadlock in legacy Hashtable writeObject()".

The problem reported is similar to one found in java.util.Vector, for
which a fix is also currently under review
(http://mail.openjdk.java.net/pipermail/core-libs-dev/2010-December/005529.html).

This fixes approach is similar to that in Vector - namely to record
the objects to be serialized within the synchronization in
Hashtable.writeObject(), so that the calls to writeObject() in that
method can be moved to beyond the synchronization, thus avoiding the
possibility of deadlock.

(The calls to writeObject() - for the keys and value of the
hashtable's entries - are the last activity within
Hashtable.writeObject(), so no additional calls need be delayed).

The keys and values to be written out are recorded in a stack of Entry
objects, created specially for the task to form a local single-linked
list of entries, which is then traversed outside the synchronized
block to make the calls to s.writeObject() with each entry's key and
value.

I believe this is the most efficient means of addressing the problem -
any suggestions on how to further improve it gratefully received.

As the order of writing out the entries has effectively changed from
FIFO to LIFO (as it uses a stack), I have reversed the order the code
traverses the table (from low index to high).

(I note that the API doc explicitly gives no guarantees as to the
order the entries are serialized. Nevertheless, as the initial
traversal order was the more uncommon of the two forms, I suspect that
there might be some deeper reasoning as to why it was done that way,
hence why I've tried to retain its spirit).

Any reviews or comment gratefully received,
Thanks,
Neil

PS:
The evaluation of this bug suggests that java.util.Random and
java.util.PropertyPermission may also be susceptible to serialization
deadlock (ie. in addition to Vector and Hashtable).

I do not believe this to be case: whilst their writeObject() methods
are synchronized, neither class needs to call
ObjectOutputStream.writeObject() within them, so the possibility of
deadlock there cannot arise.

PPS:
I notice the API javadoc for PropertyPermission does not give its
serialized form (and the "See Also:" section), even though it is
Serializable, which strikes me as unusual.

I suppose it has no serializable fields of its own (over that which
BasicPermission has), but I'm sure I've seen other serializable
classes with no fields having "serialized form" entries.

--
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU


6927486.export
Description: Binary data