srowen commented on a change in pull request #31942:
URL: https://github.com/apache/spark/pull/31942#discussion_r600914198



##########
File path: 
common/network-common/src/main/java/org/apache/spark/network/client/TransportResponseHandler.java
##########
@@ -188,6 +188,7 @@ public void handle(ResponseMessage message) throws 
Exception {
       if (listener == null) {

Review comment:
       While we're here, I wonder if it's better to extend the whole 
try-finally block around this else-if branch? what if 
outstandingRpcs.get/remove fails? highly unlikely I guess so any 'leak' is 
trivial.
   
   But what about the same problem around line 168 and 172? there's no 
try-finally around onSuccess, which could fail?
   
   Do all of these messages really need to be release()-ed? meaning, should 
.release() happen in all branches?




-- 
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]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to