Skip to content

Release buffers in netty test #126744

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

Merged
merged 5 commits into from
Apr 14, 2025
Merged

Release buffers in netty test #126744

merged 5 commits into from
Apr 14, 2025

Conversation

mhl-b
Copy link
Contributor

@mhl-b mhl-b commented Apr 12, 2025

Release leaking buffers in Netty4HttpHeaderValidatorTests.

Closes #126743
Closes #126745
Closes #126749

@mhl-b mhl-b added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Network Http and internode communication implementations Team:Distributed Coordination Meta label for Distributed Coordination team v9.1.0 labels Apr 12, 2025
@mhl-b mhl-b requested a review from DaveCTurner April 12, 2025 07:02
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM with optional suggestions

Comment on lines 143 to 146
var lastContent = channel.readInbound();
assertTrue(lastContent instanceof LastHttpContent);
((LastHttpContent) lastContent).release();

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, in one line (with slightly nicer failure message too):

Suggested change
var lastContent = channel.readInbound();
assertTrue(lastContent instanceof LastHttpContent);
((LastHttpContent) lastContent).release();
asInstanceOf(LastHttpContent.class, channel.readInbound()).release();

Comment on lines 165 to 167
var fullReq = channel.readInbound();
assertTrue(fullReq instanceof FullHttpRequest);
((FullHttpRequest) fullReq).release();
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise

Suggested change
var fullReq = channel.readInbound();
assertTrue(fullReq instanceof FullHttpRequest);
((FullHttpRequest) fullReq).release();
asInstanceOf(FullHttpRequest.class, channel.readInbound()).release();

@mhl-b mhl-b merged commit f9f3def into elastic:main Apr 14, 2025
17 checks passed
brianseeders added a commit to brianseeders/elasticsearch that referenced this pull request Apr 17, 2025
bcully added a commit to bcully/elasticsearch that referenced this pull request Apr 17, 2025
bcully pushed a commit that referenced this pull request Apr 17, 2025
…ipeline (#127030)

* Revert "Release buffers in netty test (#126744)"

This reverts commit f9f3def.

* Revert "Add flow-control and remove auto-read in netty4 HTTP pipeline (#126441)"

This reverts commit c8805b8.
ankikuma added a commit to ankikuma/elasticsearch that referenced this pull request Apr 17, 2025
This reverts commit f9f3def.

Revert Netty4HttpHeaderValidatorTests.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Network Http and internode communication implementations Team:Distributed Coordination Meta label for Distributed Coordination team >test Issues or PRs that are addressing/adding tests v9.1.0
Projects
None yet
3 participants