RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

Martin Buchholz-3
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

Stuart Marks
On 12/1/17 4:42 PM, Martin Buchholz wrote:
>     1. JDK-8192935 <https://bugs.openjdk.java.net/browse/JDK-8192935>
>
> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/EnumSet-SerializationProxy/EnumSet-SerializationProxy.patch

--- a/src/java.base/share/classes/java/util/EnumSet.java
+++ b/src/java.base/share/classes/java/util/EnumSet.java
@@ -75,7 +75,6 @@
   * @author Josh Bloch
   * @since 1.5
   * @see EnumMap
- * @serial exclude
   */

I suspect you're following other examples in the JDK that include the serial
form documentation for a class that uses a serialization proxy, but I think this
is a mistake. It's a mistake because this will cause EnumSet to appear in
serialized-form.html, but EnumSet actually should *never* appear in any
serialized byte stream. This is quite confusing. I think those other places
should be fixed, instead.

Instead of including EnumSet itself in the serialized-form.html, how about
restoring its "@serial exclude" and then putting a link directly from the
EnumSet class doc to the EnumSet.SerializationProxy serial form doc? Something like

  * <p>When an instance of this class is serialized, it is replaced with the

  * serial form of an instance of

  * <a href="../../serialized-form.html#java.util.EnumSet.SerializationProxy">

  * {@code EnumSet.SerializationProxy}</a>.


The changes to EnumSet.SerializationProxy class are good.

s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

Martin Buchholz-3
Interesting.  You are trying to define a new Best Practice for the
Serialization Proxy Pattern.

Serialization is weird/broken in many ways - one of its weirdnesses is
documenting behavior indirectly by publishing javadoc for private (!)
methods (and fixing that would be a huge project (and I'm definitely not
signing up to fix serialization!)).  So the writeReplace and readResolve
methods together describe how to serialize and deserialize these classes.
But the methods are in different classes!  Further, there's the serial spec
for readObject which ensures that bogus serial forms are rejected, which is
another reason to publish a serial form spec for EnumSet.  There's also a
reasonable expectation that public Serializable classes have an entry in
the serialized form page.  So I'd like to go with what I've got.

http://cr.openjdk.java.net/~martin/webrevs/jdk/EnumSet-SerializationProxy/

(I had to modify the BogusEnumSet test, probably due to my added
"transient")


On Fri, Dec 1, 2017 at 5:20 PM, Stuart Marks <[hidden email]>
wrote:

> On 12/1/17 4:42 PM, Martin Buchholz wrote:
>
>>     1. JDK-8192935 <https://bugs.openjdk.java.net/browse/JDK-8192935>
>>
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/EnumSet
>> -SerializationProxy/EnumSet-SerializationProxy.patch
>>
>
> --- a/src/java.base/share/classes/java/util/EnumSet.java
> +++ b/src/java.base/share/classes/java/util/EnumSet.java
> @@ -75,7 +75,6 @@
>   * @author Josh Bloch
>   * @since 1.5
>   * @see EnumMap
> - * @serial exclude
>   */
>
> I suspect you're following other examples in the JDK that include the
> serial form documentation for a class that uses a serialization proxy, but
> I think this is a mistake. It's a mistake because this will cause EnumSet
> to appear in serialized-form.html, but EnumSet actually should *never*
> appear in any serialized byte stream. This is quite confusing. I think
> those other places should be fixed, instead.
>
> Instead of including EnumSet itself in the serialized-form.html, how about
> restoring its "@serial exclude" and then putting a link directly from the
> EnumSet class doc to the EnumSet.SerializationProxy serial form doc?
> Something like
>
>  * <p>When an instance of this class is serialized, it is replaced with
> the
>  * serial form of an instance of
>  * <a href="../../serialized-form.html#java.util.EnumSet.SerializationProxy">
>
>  * {@code EnumSet.SerializationProxy}</a>.
>
> The changes to EnumSet.SerializationProxy class are good.
>
> s'marks
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

roger riggs
Hi Martin,

The java.time APIs refined the pattern used for Serialization proxies to
document the relationship between
the original class and its serialization proxies methods.

It is important the EnumSet appear in the serialized form to document
the behavior
that it should no appear in the stream and to provide links to the
expected serial proxy type
and its behaviors.  The SerializationProxy (though a private class) is
part of the public API for serialization.

EnumSet should have a serialVersion uid anyway; to avoid the kind of
touchup needed in the test.
(And you would not need to suppress the warning). There is no harm in it
having a fixed suid.

452: SerializationProxy.readResolve doesn't need the comment about
elementType.cast
since EnumSet.add method checks each element that is added.

Regards, Roger


On 12/4/2017 2:45 PM, Martin Buchholz wrote:

> Interesting.  You are trying to define a new Best Practice for the
> Serialization Proxy Pattern.
>
> Serialization is weird/broken in many ways - one of its weirdnesses is
> documenting behavior indirectly by publishing javadoc for private (!)
> methods (and fixing that would be a huge project (and I'm definitely not
> signing up to fix serialization!)).  So the writeReplace and readResolve
> methods together describe how to serialize and deserialize these classes.
> But the methods are in different classes!  Further, there's the serial spec
> for readObject which ensures that bogus serial forms are rejected, which is
> another reason to publish a serial form spec for EnumSet.  There's also a
> reasonable expectation that public Serializable classes have an entry in
> the serialized form page.  So I'd like to go with what I've got.
>
> http://cr.openjdk.java.net/~martin/webrevs/jdk/EnumSet-SerializationProxy/
>
> (I had to modify the BogusEnumSet test, probably due to my added
> "transient")
>
>
> On Fri, Dec 1, 2017 at 5:20 PM, Stuart Marks <[hidden email]>
> wrote:
>
>> On 12/1/17 4:42 PM, Martin Buchholz wrote:
>>
>>>      1. JDK-8192935 <https://bugs.openjdk.java.net/browse/JDK-8192935>
>>>
>>> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/EnumSet
>>> -SerializationProxy/EnumSet-SerializationProxy.patch
>>>
>> --- a/src/java.base/share/classes/java/util/EnumSet.java
>> +++ b/src/java.base/share/classes/java/util/EnumSet.java
>> @@ -75,7 +75,6 @@
>>    * @author Josh Bloch
>>    * @since 1.5
>>    * @see EnumMap
>> - * @serial exclude
>>    */
>>
>> I suspect you're following other examples in the JDK that include the
>> serial form documentation for a class that uses a serialization proxy, but
>> I think this is a mistake. It's a mistake because this will cause EnumSet
>> to appear in serialized-form.html, but EnumSet actually should *never*
>> appear in any serialized byte stream. This is quite confusing. I think
>> those other places should be fixed, instead.
>>
>> Instead of including EnumSet itself in the serialized-form.html, how about
>> restoring its "@serial exclude" and then putting a link directly from the
>> EnumSet class doc to the EnumSet.SerializationProxy serial form doc?
>> Something like
>>
>>   * <p>When an instance of this class is serialized, it is replaced with
>> the
>>   * serial form of an instance of
>>   * <a href="../../serialized-form.html#java.util.EnumSet.SerializationProxy">
>>
>>   * {@code EnumSet.SerializationProxy}</a>.
>>
>> The changes to EnumSet.SerializationProxy class are good.
>>
>> s'marks
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

roger riggs
Correction below.

On 12/4/2017 3:12 PM, Roger Riggs wrote:

> Hi Martin,
>
> The java.time APIs refined the pattern used for Serialization proxies
> to document the relationship between
> the original class and its serialization proxies methods.
>
> It is important the EnumSet appear in the serialized form to document
> the behavior
> that it should no appear in the stream and to provide links to the
> expected serial proxy type
> and its behaviors.  The SerializationProxy (though a private class) is
> part of the public API for serialization.
>
> EnumSet should have a serialVersion uid anyway; to avoid the kind of
> touchup needed in the test.
> (And you would not need to suppress the warning). There is no harm in
> it having a fixed suid.
Never mind, the proxy's suid has nothing to do with the BogusEnumSet suid.

>
> 452: SerializationProxy.readResolve doesn't need the comment about
> elementType.cast
> since EnumSet.add method checks each element that is added.
>
> Regards, Roger
>
>
> On 12/4/2017 2:45 PM, Martin Buchholz wrote:
>> Interesting.  You are trying to define a new Best Practice for the
>> Serialization Proxy Pattern.
>>
>> Serialization is weird/broken in many ways - one of its weirdnesses is
>> documenting behavior indirectly by publishing javadoc for private (!)
>> methods (and fixing that would be a huge project (and I'm definitely not
>> signing up to fix serialization!)).  So the writeReplace and readResolve
>> methods together describe how to serialize and deserialize these
>> classes.
>> But the methods are in different classes!  Further, there's the
>> serial spec
>> for readObject which ensures that bogus serial forms are rejected,
>> which is
>> another reason to publish a serial form spec for EnumSet. There's also a
>> reasonable expectation that public Serializable classes have an entry in
>> the serialized form page.  So I'd like to go with what I've got.
>>
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/EnumSet-SerializationProxy/ 
>>
>>
>> (I had to modify the BogusEnumSet test, probably due to my added
>> "transient")
>>
>>
>> On Fri, Dec 1, 2017 at 5:20 PM, Stuart Marks <[hidden email]>
>> wrote:
>>
>>> On 12/1/17 4:42 PM, Martin Buchholz wrote:
>>>
>>>>      1. JDK-8192935 <https://bugs.openjdk.java.net/browse/JDK-8192935>
>>>>
>>>> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/EnumSet
>>>> -SerializationProxy/EnumSet-SerializationProxy.patch
>>>>
>>> --- a/src/java.base/share/classes/java/util/EnumSet.java
>>> +++ b/src/java.base/share/classes/java/util/EnumSet.java
>>> @@ -75,7 +75,6 @@
>>>    * @author Josh Bloch
>>>    * @since 1.5
>>>    * @see EnumMap
>>> - * @serial exclude
>>>    */
>>>
>>> I suspect you're following other examples in the JDK that include the
>>> serial form documentation for a class that uses a serialization
>>> proxy, but
>>> I think this is a mistake. It's a mistake because this will cause
>>> EnumSet
>>> to appear in serialized-form.html, but EnumSet actually should *never*
>>> appear in any serialized byte stream. This is quite confusing. I think
>>> those other places should be fixed, instead.
>>>
>>> Instead of including EnumSet itself in the serialized-form.html, how
>>> about
>>> restoring its "@serial exclude" and then putting a link directly
>>> from the
>>> EnumSet class doc to the EnumSet.SerializationProxy serial form doc?
>>> Something like
>>>
>>>   * <p>When an instance of this class is serialized, it is replaced
>>> with
>>> the
>>>   * serial form of an instance of
>>>   * <a
>>> href="../../serialized-form.html#java.util.EnumSet.SerializationProxy">
>>>
>>>   * {@code EnumSet.SerializationProxy}</a>.
>>>
>>> The changes to EnumSet.SerializationProxy class are good.
>>>
>>> s'marks
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

Martin Buchholz-3
In reply to this post by roger riggs
Hi Roger,

On Mon, Dec 4, 2017 at 12:12 PM, Roger Riggs <[hidden email]> wrote:

> Hi Martin,
>
> The java.time APIs refined the pattern used for Serialization proxies to
> document the relationship between
> the original class and its serialization proxies methods.
>

Right.  I was aware of the effort that java.time people put into their
serialization code.  They did a good job, and my proposed change makes it
more like theirs (but even more like the ones in j.u.c.a.)


> 452: SerializationProxy.readResolve doesn't need the comment about
> elementType.cast
> since EnumSet.add method checks each element that is added.
>

You may be right, but I was only preserving the original comment, and
removing it should be a separate change (I leave it to you!).
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

roger riggs
Keep the comment, I didn't notice it had only been relocated.

Thanks,

On 12/4/2017 3:36 PM, Martin Buchholz wrote:

> Hi Roger,
>
> On Mon, Dec 4, 2017 at 12:12 PM, Roger Riggs <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi Martin,
>
>     The java.time APIs refined the pattern used for Serialization
>     proxies to document the relationship between
>     the original class and its serialization proxies methods.
>
>
> Right.  I was aware of the effort that java.time people put into their
> serialization code.  They did a good job, and my proposed change makes
> it more like theirs (but even more like the ones in j.u.c.a.)
:)
>
>     452: SerializationProxy.readResolve doesn't need the comment about
>     elementType.cast
>     since EnumSet.add method checks each element that is added.
>
>
> You may be right, but I was only preserving the original comment, and
> removing it should be a separate change (I leave it to you!).

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

Stuart Marks
In reply to this post by Martin Buchholz-3


On 12/4/17 12:36 PM, Martin Buchholz wrote:
> On Mon, Dec 4, 2017 at 12:12 PM, Roger Riggs <[hidden email]> wrote:
>> The java.time APIs refined the pattern used for Serialization proxies to
>> document the relationship between
>> the original class and its serialization proxies methods.
>>
>
> Right.  I was aware of the effort that java.time people put into their
> serialization code.  They did a good job, and my proposed change makes it
> more like theirs (but even more like the ones in j.u.c.a.)

Hi Martin, Roger,

I disagree that the java.time approach is a Best Practice (capitalized), as
Martin said, for dealing with documentation of serial proxies. It's a workaround
for lack of support for this pattern in the javadoc tool.

Having the redeclare all the fields of EnumSet as transient is evidence of this.
(IMO the java.time classes are worse, as they include the fields of objects that
are replaced by the proxy.)

Having the original class appear in the serialized form documentation is a bug,
since it never appears in the serial form. It sort-of helps define the
relationship between the original class and the proxy, but in a confusing,
roundabout way.

If you don't like my alternative, fine; it has its own set of tradeoffs that
might be net positive or negative. If you want to proceed with the current
approach, then I won't stand in the way. At the very least there should be some
boilerplate added to EnumSet that makes it clear that EnumSet itself never
appears in the serial form.

s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

roger riggs
Hi Stuart,

On 12/4/2017 4:23 PM, Stuart Marks wrote:

>
>
> On 12/4/17 12:36 PM, Martin Buchholz wrote:
>> On Mon, Dec 4, 2017 at 12:12 PM, Roger Riggs <[hidden email]>
>> wrote:
>>> The java.time APIs refined the pattern used for Serialization
>>> proxies to
>>> document the relationship between
>>> the original class and its serialization proxies methods.
>>>
>>
>> Right.  I was aware of the effort that java.time people put into their
>> serialization code.  They did a good job, and my proposed change
>> makes it
>> more like theirs (but even more like the ones in j.u.c.a.)
>
> Hi Martin, Roger,
>
> I disagree that the java.time approach is a Best Practice
> (capitalized), as Martin said, for dealing with documentation of
> serial proxies. It's a workaround for lack of support for this pattern
> in the javadoc tool.
I wouldn't argue with javadoc tool being improved, but with the tools
available...
>
> Having the redeclare all the fields of EnumSet as transient is
> evidence of this. (IMO the java.time classes are worse, as they
> include the fields of objects that are replaced by the proxy.)
Another alternative is to define ObjectStreamField[]
serialPersistentFields = {};

>
> Having the original class appear in the serialized form documentation
> is a bug, since it never appears in the serial form. It sort-of helps
> define the relationship between the original class and the proxy, but
> in a confusing, roundabout way.
I disagree here since it implements Serializable and needs to be indexed
as such.
>
> If you don't like my alternative, fine; it has its own set of
> tradeoffs that might be net positive or negative. If you want to
> proceed with the current approach, then I won't stand in the way. At
> the very least there should be some boilerplate added to EnumSet that
> makes it clear that EnumSet itself never appears in the serial form.
I don't disagree, there are many things that could be improved.

Roger


Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

Martin Buchholz-3
On Mon, Dec 4, 2017 at 1:46 PM, Roger Riggs <[hidden email]> wrote:

>
> If you don't like my alternative, fine; it has its own set of tradeoffs
> that might be net positive or negative. If you want to proceed with the
> current approach, then I won't stand in the way. At the very least there
> should be some boilerplate added to EnumSet that makes it clear that
> EnumSet itself never appears in the serial form.
>
> I don't disagree, there are many things that could be improved.
>

I only volunteered to bring EnumSet (as the poster child for the
Serialization Proxy Pattern) into a no-worse state than other classes
implementing the pattern.  The doc of the writeReplace and readObject
methods is pretty good implicit documentation that the pattern applies
here.  Serialization overall remains as deeply flawed as ever.

I still plan to submit what I have now.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

Stuart Marks
>>     If you don't like my alternative, fine; it has its own set of tradeoffs
>>     that might be net positive or negative. If you want to proceed with the
>>     current approach, then I won't stand in the way. At the very least there
>>     should be some boilerplate added to EnumSet that makes it clear that
>>     EnumSet itself never appears in the serial form.
>     I don't disagree, there are many things that could be improved.
>
> I only volunteered to bring EnumSet (as the poster child for the Serialization
> Proxy Pattern) into a no-worse state than other classes implementing the
> pattern.  The doc of the writeReplace and readObject methods is pretty good
> implicit documentation that the pattern applies here.  Serialization overall
> remains as deeply flawed as ever.
>
> I still plan to submit what I have now.

Thanks for volunteering. It goes to show that no good deed goes unpunished. :-)

To close the loop on this, I think what you have is acceptable. I also think
that "no-worse state" is a better characterization than "Best Practice," which
seems to imply that no further improvement is possible or necessary. And
finally, Jon Gibbons has filed JDK-8193019 to cover future javadoc enhancements
to better support serialization.

s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc

Martin Buchholz-3
Pushed.  In future I will use the phrase "Best of a Bad Lot Practice".

On Mon, Dec 4, 2017 at 4:20 PM, Stuart Marks <[hidden email]>
wrote:

>     If you don't like my alternative, fine; it has its own set of tradeoffs
>>>     that might be net positive or negative. If you want to proceed with
>>> the
>>>     current approach, then I won't stand in the way. At the very least
>>> there
>>>     should be some boilerplate added to EnumSet that makes it clear that
>>>     EnumSet itself never appears in the serial form.
>>>
>>     I don't disagree, there are many things that could be improved.
>>
>> I only volunteered to bring EnumSet (as the poster child for the
>> Serialization Proxy Pattern) into a no-worse state than other classes
>> implementing the pattern.  The doc of the writeReplace and readObject
>> methods is pretty good implicit documentation that the pattern applies
>> here.  Serialization overall remains as deeply flawed as ever.
>>
>> I still plan to submit what I have now.
>>
>
> Thanks for volunteering. It goes to show that no good deed goes
> unpunished. :-)
>
> To close the loop on this, I think what you have is acceptable. I also
> think that "no-worse state" is a better characterization than "Best
> Practice," which seems to imply that no further improvement is possible or
> necessary. And finally, Jon Gibbons has filed JDK-8193019 to cover future
> javadoc enhancements to better support serialization.
>
> s'marks
>