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

UNDERTOW-2143 Deflaters operate directly on ByteBuffers #1367

Merged
merged 6 commits into from
Oct 12, 2022

Conversation

carterkozak
Copy link
Contributor

@carterkozak carterkozak commented Aug 23, 2022

@carterkozak carterkozak marked this pull request as ready for review August 23, 2022 20:54
Comment on lines -125 to -131
//we may already have some input, if so compress it
if (!deflater.needsInput()) {
deflateData(false);
if (!deflater.needsInput()) {
return 0;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case is no longer possible because we deflate from a source buffer and return the difference in position (the amount the deflater has consumed), and set an EMPTY buffer to prevent ourselves from leaking references to that buffer. As such, before and after a call to write, the deflater always needs input.

@fl4via fl4via self-requested a review September 18, 2022 12:05
@fl4via fl4via added the bug fix Contains bug fix(es) label Sep 18, 2022
@@ -556,7 +554,6 @@ private void freeBuffer() {
deflater = null;
pooledObject.close();
}
IoUtils.safeClose(additionalBuffer);
Copy link
Member

Choose a reason for hiding this comment

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

I'm missing the points where the buffers are closed? I think you need to close, for example, trailerBuffer here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trailerBuffer is no longer a pooled buffer, rather the result of ByteBuffer.wrap( on the byte-array result of getTrailer()

@fl4via fl4via added the waiting PR update Awaiting PR update(s) from contributor before merging label Oct 11, 2022
@fl4via fl4via removed the waiting PR update Awaiting PR update(s) from contributor before merging label Oct 12, 2022
@fl4via fl4via added next release This PR will be merged before next release or has already been merged (for payload double check) waiting CI check Ready to be merged but waiting for CI check and removed waiting CI check Ready to be merged but waiting for CI check labels Oct 12, 2022
@fl4via fl4via merged commit a5ca754 into undertow-io:master Oct 12, 2022
@fl4via fl4via removed the next release This PR will be merged before next release or has already been merged (for payload double check) label Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Contains bug fix(es)
Projects
None yet
2 participants