-
Notifications
You must be signed in to change notification settings - Fork 153
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.
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
[#1472][FOLLOWUP] fix(client): Fix IllegalReferenceCountException issues when exceptions happened in clientReadHandler.readShuffleData() #1536
Conversation
…on issues when exceptions happened in clientReadHandler.readShuffleData()
Test Results2 432 files - 5 2 432 suites - 5 4h 39m 36s ⏱️ - 2m 44s For more details on these failures, see this check. Results for commit fc84871. ± Comparison against base commit 59aa30d. |
@@ -269,6 +269,10 @@ private int read() { | |||
// because PlatformDependent.freeDirectBuffer can only release the ByteBuffer with cleaner. | |||
if (sdr != null) { | |||
sdr.release(); | |||
// We set sdr to null here to prevent IllegalReferenceCountException that could occur | |||
// if sdr.release() is called multiple times in the close() method, | |||
// when an exception is thrown by clientReadHandler.readShuffleData(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get why this will trigger the issue. sdr will be reassign in the Line 277.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when an exception is thrown by clientReadHandler.readShuffleData().
An exception is thrown from line 277. So sdr
will not be reassigned successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I know.
CI is unstable, I don't think the failure is related to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
What changes were proposed in this pull request?
Fix IllegalReferenceCountException issues when exceptions happened in clientReadHandler.readShuffleData().
Why are the changes needed?
A follow-up PR for: #1522
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing UTs.