[GitHub] [geode] kirklund commented on a change in pull request #6943: GEODE-9641: Remove unnecessary allocation of HeapDataOutputStream

[GitBox <gi...@apache.org>]

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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org