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

Fix memory leak in http pipelining #30815

Closed
wants to merge 1 commit into from

Conversation

Tim-Brooks
Copy link
Contributor

@Tim-Brooks Tim-Brooks commented May 23, 2018

This is related to #30801. When we call the http pipelining
aggregator on an inbound request, we retain the netty request. However,
this is unnecessary as the pipelining aggregator does not store the
request. This worked in the past as we manually release the request and
netty internally automatically releases the request. At this point we do
not implement the ref counter interface after the pipelining step which
means that netty is no longer automatically handling this second retain.

This commit removes that retain.

This is related to elastic#30801. When we calling the http pipelining
aggregator on an inbound request, we retain the netty request. However,
this is unnecessary as the pipelining aggregator does not store the
request. This worked in the past as we manually release the request and
netty internally automatically releases the request. At this point we do
not implement the ref counter interface after the pipelining step which
means that netty is no longer automatically handling this second retain.

This commit removes that retain.
@Tim-Brooks Tim-Brooks added >bug review :Distributed Coordination/Network Http and internode communication implementations v7.0.0 labels May 23, 2018
@Tim-Brooks Tim-Brooks requested review from s1monw and jasontedor May 23, 2018 14:54
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@Tim-Brooks
Copy link
Contributor Author

Closed in favor of #30820.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Coordination/Network Http and internode communication implementations v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants