JGRP-2242 Narrow set of thrown exceptions in Streamable and Marshaller method signatures.

[pferraro]

https://issues.jboss.org/browse/JGRP-2242

Now that 4.0.x was branched, perhaps this is ready for inclusion?

[belaban]

What's the rationale again for changing almost all of JGroups? :-)

[pferraro]

@belaban This removes the need for consumers of these methods to consistently handle a non-specific java.lang.Exception - particularly useful when adapting to other marshalling frameworks.

Here's the original PR for context: #372

[belaban]

Do you have an example (code)?

[pferraro]

Off the top of my head:
https://github.com/wildfly/wildfly/blob/master/clustering/server/src/main/java/org/wildfly/clustering/server/group/AddressSerializer.java#L49
https://github.com/wildfly/wildfly/blob/master/clustering/server/src/main/java/org/wildfly/clustering/server/group/AddressSerializer.java#L61
https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/remoting/transport/jgroups/JGroupsAddress.java#L69
https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/remoting/transport/jgroups/JGroupsAddress.java#L78
https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/remoting/transport/jgroups/JGroupsTopologyAwareAddress.java#L131
https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/remoting/transport/jgroups/JGroupsTopologyAwareAddress.java#L140

[belaban]

Hmm... so your code throws an IOException... But [1] is inconsistent: it throw IOException on write() and also on read(), but other code throws IOException, ClassNotFoundException on read().

I'll think about this

[1] https://github.com/wildfly/wildfly/blob/master/clustering/server/src/main/java/org/wildfly/clustering/server/group/AddressSerializer.java#L60

[pferraro]

These are pretty standard exception types to throw when reading/writing to/from an java.io.ObjectInput/ObjectOutut stream. See:
https://docs.oracle.com/javase/8/docs/api/java/io/Externalizable.html
https://github.com/wildfly/wildfly/blob/master/clustering/marshalling/api/src/main/java/org/wildfly/clustering/marshalling/Externalizer.java
https://github.com/infinispan/infinispan/blob/master/commons/src/main/java/org/infinispan/commons/marshall/Externalizer.java
https://github.com/jboss-remoting/jboss-marshalling/blob/master/api/src/main/java/org/jboss/marshalling/Externalizer.java

This is because ObjectInput.readObject() itself throws both an IOException or a ClassNotFoundException, if the class descriptor from the stream cannot be loaded.
https://docs.oracle.com/javase/8/docs/api/java/io/ObjectInput.html#readObject--
Whereas ObjectOutput.writeObject(...) only ever throws an IOException (i.e. it cannot be the case that the class was not found, if we already have an instance of the class)
https://docs.oracle.com/javase/8/docs/api/java/io/ObjectOutput.html#writeObject-java.lang.Object-

[belaban]

OK, so both methods will only throw an IOException.
I'll change this in 4.1.0

[rhusar]

LGTM

[belaban]

I'll take a look, too. I'm currently busy with the port to Quarkus, but will take a look early next week.

[belaban]

I don't like having to add a ClassNoutFoundException to the readXXX() methods. As in your examples above, I'd like to add this exception only to methods (e.g. Util.readAddress()) that possibly load classes.
Adding this to Streamable.readFrom() is not desired.
WDYT?

[belaban]

These are pretty standard exception types to throw when reading/writing to/from an java.io.ObjectInput/ObjectOutut stream. See:
https://docs.oracle.com/javase/8/docs/api/java/io/Externalizable.html
https://github.com/wildfly/wildfly/blob/master/clustering/marshalling/api/src/main/java/org/wildfly/clustering/marshalling/Externalizer.java
https://github.com/infinispan/infinispan/blob/master/commons/src/main/java/org/infinispan/commons/marshall/Externalizer.java
https://github.com/jboss-remoting/jboss-marshalling/blob/master/api/src/main/java/org/jboss/marshalling/Externalizer.java

This is because ObjectInput.readObject() itself throws both an IOException or a ClassNotFoundException, if the class descriptor from the stream cannot be loaded.
https://docs.oracle.com/javase/8/docs/api/java/io/ObjectInput.html#readObject--
Whereas ObjectOutput.writeObject(...) only ever throws an IOException (i.e. it cannot be the case that the class was not found, if we already have an instance of the class)
https://docs.oracle.com/javase/8/docs/api/java/io/ObjectOutput.html#writeObject-java.lang.Object-

While that is correct, in most cases, JGroups does not read/write Objects, but simply - primitive - types.

[pferraro]

I'd like to add this exception only to methods (e.g. Util.readAddress()) that possibly load classes.

That would be a welcome improvement.

[pferraro]

While that is correct, in most cases, JGroups does not read/write Objects, but simply - primitive - types.

For internal messages, yes. However, the message I would be most concerned about are those created and sent by the user. When these messages are serialized, you wrap the DataInput/DataOutput with an ObjectOutputStream/ObjectInputStream whenever you encounter a Serializable object.
e.g.

• https://github.com/belaban/JGroups/blob/master/src/org/jgroups/util/Util.java#L511
• https://github.com/belaban/JGroups/blob/master/src/org/jgroups/util/Util.java#L551
• https://github.com/belaban/JGroups/blob/master/src/org/jgroups/util/Util.java#L578
• https://github.com/belaban/JGroups/blob/master/src/org/jgroups/util/Util.java#L670
• https://github.com/belaban/JGroups/blob/master/src/org/jgroups/util/Util.java#L750

I understand why you want to avoid the use of ObjectOutput/ObjectInput whenever possible, as it adds quite a bit of overhead to the stream (~50-60 bytes). The current approach is fine, so long as you only ever write at most a single serializable object to the stream. However, as soon as you write multiple serializable objects to a stream, you'll multiply this overhead unnecessarily instead of incurring it only once. While it is reasonable to assume that most user created messages will probably only contain a single serializable object, a batch of those same messages (which are serialized to a single stream) incur accumulated overhead that could otherwise be avoided if we were writing to a single ObjectOutput stream.

[belaban]

OK. I'll accept your PR, but will make additional modifications, as not all methods (e.g. in Util) have the correct exceptions.

[belaban]

Pushed to master. Let me know if this works for you

[pferraro]

That works. Thanks!