-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Safely fail queued writes in Netty4WriteThrottlingHandler #90978
base: main
Are you sure you want to change the base?
Safely fail queued writes in Netty4WriteThrottlingHandler #90978
Conversation
Add the hook to fail queued writes to disconnect and channel close, as is done by `SslHandler`. Maybe this explains random leaks on unstable connections.
Pinging @elastic/es-distributed (Team:Distributed) |
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 hope we can add a bit more testing.
|
||
@Override | ||
public void handlerRemoved(ChannelHandlerContext ctx) throws Exception { | ||
failQueuedWrites(); |
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.
Can we add testing of the 3 methods and their expected behavior?
Add the hook to fail queued writes to disconnect and channel close, as is done by
SslHandler
. Maybe this explains random leaks on unstable connections, though to be honest I have no proof of this yet.It seems like if anything this change makes it so some memory is released earlier which is always a plus even if it doesn't catch the leak. Also, it rules out one route to leaks.