Skip to content

[SPARK-34834][NETWORK] Fix a potential Netty memory leak in TransportResponseHandler. #31942

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
weixiuli wants to merge 1 commit into from
Closed

[SPARK-34834][NETWORK] Fix a potential Netty memory leak in TransportResponseHandler. #31942

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...etwork-common/src/main/java/org/apache/spark/network/client/TransportResponseHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ public void handle(ResponseMessage message) throws Exception {
if (listener == null) {

@srowen srowen Mar 24, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@weixiuli weixiuli Mar 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. , Yes , the outstandingRpcs.get/remove can not fail, so we should not try-finally block around this else-if branch here, which may Improve performance.

But what about the same problem around line 168 and 172? there's no try-finally around onSuccess, which could fail?, i think we should also try-finally around onSuccess around line 168 and 172, do you think so ?

Do all of these messages really need to be release()-ed? meaning, should .release() happen in all branches?,It should be released, although this scenario is not easy to happen.

@srowen srowen Mar 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wonder if it would be simpler to wrap a lot more in one big try-finally and release the thing in all cases?

logger.warn("Ignoring response for RPC {} from {} ({} bytes) since it is not outstanding",
resp.requestId, getRemoteAddress(channel), resp.body().size());
resp.body().release();
Comment thread
srowen marked this conversation as resolved.
} else {
outstandingRpcs.remove(resp.requestId);
try {
Expand Down