-
Notifications
You must be signed in to change notification settings - Fork 986
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
[UNDERTOW-2403] Fix race condition in read from buffer at ServletInpu… #1602
base: main
Are you sure you want to change the base?
Conversation
…tStreamImpl: prevent a NPE from being thrown to invoker in case input stream is closed by another thread (which can happen typically in timeouts) Signed-off-by: Flavia Rainone <frainone@redhat.com>
} | ||
} | ||
} | ||
} | ||
|
||
private void closePoolIfNotNull() { | ||
try { | ||
if (pooled != null) { |
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.
ONly thing Id consider is to have local var? unless thats calculated choice to sometimes pay price of exception, rather than constant cost of local var and assignment.
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.
@fl4via ^^
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.
We will pay the price of the NPE anyway, but, yes, I think adding the local var is a good idea because it will make the exception less likely.
if (pooled != null) {
final PooledByteBuffer pooled = this.pooled; // we can still read a null value here, hence, NPE when closing
this.pooled = null;
pooled.close(); // marginal risk of NPE here
}
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.
As a secod thought, will it make it less likely, if the time spent seting it to a variable might be the same time it takes to invoke close directly? Or is it not? I'll try to see if I can find some information on this. But the code I suggested above might have the same statistical risk of getting the NPE as the one we have without the local variable.
Im going to trigger CI once more. Most likely this either needs to disable on mac or expect failure. Most likely we should have some sort of OS specific annotations or utility to make this kind of exclusion easier. |
…tStreamImpl: prevent a NPE from being thrown to invoker in case input stream is closed by another thread (which can happen typically in timeouts)
Jira: https://issues.redhat.com/browse/UNDERTOW-2403