Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

original-brownbear
Copy link
Member

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.

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.
@original-brownbear original-brownbear added >non-issue :Distributed/Network Http and internode communication implementations v8.6.0 labels Oct 18, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team label Oct 18, 2022
Copy link
Contributor

@henningandersen henningandersen left a 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();
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Network Http and internode communication implementations >non-issue Team:Distributed Meta label for distributed team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.