RFR: JDK-8192935 Fix EnumSet's SerializationProxy javadoc
1. JDK-8192935 <https://bugs.openjdk.java.net/browse/JDK-8192935> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/EnumSet-SerializationPr...
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-SerializationPr...
--- 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
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 <stuart.marks@oracle.com> 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
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 <stuart.marks@oracle.com> 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
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 <stuart.marks@oracle.com> 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
Hi Roger, On Mon, Dec 4, 2017 at 12:12 PM, Roger Riggs <Roger.Riggs@oracle.com> 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!).
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 <Roger.Riggs@oracle.com <mailto:Roger.Riggs@oracle.com>> 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!).
On 12/4/17 12:36 PM, Martin Buchholz wrote:
On Mon, Dec 4, 2017 at 12:12 PM, Roger Riggs <Roger.Riggs@oracle.com> 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
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 <Roger.Riggs@oracle.com> 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
On Mon, Dec 4, 2017 at 1:46 PM, Roger Riggs <Roger.Riggs@oracle.com> 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.
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
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 <stuart.marks@oracle.com> 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
participants (3)
-
Martin Buchholz
-
Roger Riggs
-
Stuart Marks