dschneider-pivotal commented on a change in pull request #6426:
URL: https://github.com/apache/geode/pull/6426#discussion_r646991268
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/ReplyMessage.java
##########
@@ -295,7 +295,14 @@ public void toData(DataOutput out,
out.writeInt(processorId);
}
if (this.returnValueIsException || this.returnValue != null) {
- DataSerializer.writeObject(this.returnValue, out);
+ try {
+ context.getSerializer().writeObject(this.returnValue, out);
+ } catch (NotSerializableException e) {
Review comment:
I think it would be better to have toData serialize returnValue to its
own data buffer (you might want to use HeapDataOutputStream) and if that
serialization fails. then change the reply to have "returnValueIsException" and
set the "returnValue" to the exception you caught during that serialization.
You need to do this before "writeByte(status)" is called since you may be
changing the status to have the exception flag set. That way the caller gets a
nice exception explaining how the reply data was not serializable. The downside
is that you have to serialize it to a temp buffer but it should have a short
lifetime and you if no exception happens then you can copy it to the stream
efficiently using HeapDataOutputStream.sendTo(DataOutput). Doing a partial
write to the DataOutput and catching the exception and then depending on that
to always give us a WriteAbortedException on the other side seems scary to me.
I'm not sure you would always get the behavior. Also catching IOExceptio
n in fromData and treating it like the other side sent us a reply exception
seems like it would lead to confusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
