kirklund commented on a change in pull request #6943:
URL: https://github.com/apache/geode/pull/6943#discussion_r724578772
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/ReplyMessage.java
##########
@@ -274,10 +274,11 @@ public void toData(DataOutput out,
SerializationContext context) throws IOException {
super.toData(out, context);
- final HeapDataOutputStream hdos =
- new HeapDataOutputStream(context.getSerializationVersion());
+ HeapDataOutputStream hdos = null;
boolean failedSerialization = false;
- if (this.returnValueIsException || this.returnValue != null) {
+ final boolean hasReturnValue = this.returnValueIsException ||
this.returnValue != null;
+ if (hasReturnValue) {
+ hdos = new HeapDataOutputStream(context.getSerializationVersion());
Review comment:
[comment] (Just a comment, not a request for changes) It would be best
to close `hdos` in a finally-block. The try-with-resource syntax is generally
the preferred syntax. If you make this change, you would probably have to do
some refactoring to extract methods or it won't be very clean.
It's safest to do refactoring like the following example if there's a decent
unit test. ReplyMessageTest exists but unfortunately, it doesn't cover much.
You could try something like this which also seems to be more readable. In
order: extracted writeStatus, extracted writeProcessProcessorId, moved hdos
into a try-with-resource block, polished up the try-with-resource block,
extracted writeReturnValue which pulled the nested try-catch block out:
```
@Override
public void toData(DataOutput out,
SerializationContext context) throws IOException {
super.toData(out, context);
boolean failedSerialization = false;
final boolean hasReturnValue = this.returnValueIsException ||
this.returnValue != null;
if (hasReturnValue) {
try (HeapDataOutputStream hdos = new
HeapDataOutputStream(context.getSerializationVersion())) {
writeReturnValue(out, context, failedSerialization, hdos);
}
} else {
writeStatus(out);
writeProcessorId(out);
}
}
private void writeReturnValue(DataOutput out, SerializationContext
context,
boolean failedSerialization, HeapDataOutputStream hdos) throws
IOException {
try {
context.getSerializer().writeObject(this.returnValue, hdos);
} catch (NotSerializableException e) {
logger.warn("Unable to serialize a reply to " +
getRecipientsDescription(), e);
failedSerialization = true;
this.returnValue = new ReplyException(e);
this.returnValueIsException = true;
}
writeStatus(out);
writeProcessorId(out);
if (failedSerialization) {
context.getSerializer().writeObject(this.returnValue, out);
} else {
hdos.sendTo(out);
}
}
private void writeProcessorId(DataOutput out) throws IOException {
if (this.processorId != 0) {
out.writeInt(processorId);
}
}
private void writeStatus(DataOutput out) throws IOException {
byte status = 0;
if (this.ignored) {
status |= IGNORED_FLAG;
}
if (this.returnValueIsException) {
status |= EXCEPTION_FLAG;
} else if (this.returnValue != null) {
status |= OBJECT_FLAG;
}
if (this.processorId != 0) {
status |= PROCESSOR_ID_FLAG;
}
if (this.closed) {
status |= CLOSED_FLAG;
}
if (this.internal) {
status |= INTERNAL_FLAG;
}
out.writeByte(status);
}
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
