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 #6562 last written bytebuffer #6563

Merged
merged 3 commits into from Aug 3, 2021

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Jul 30, 2021

Fixes #6562 the last written bytebuffer calculation.
Also fixed an associated issue with unnecessary flush of an empty when last calculation already signalled last.

Fixes #6562 the last written bytebuffer calculation.
Also fixed an associated issue with unnecessary flush of an empty when last calculation already signalled last.

The code coverage is not complete, so more tests are needed for this use case. Also strange that `write(ByteBuffer)` does not appear to every commence aggregation?
@gregw gregw self-assigned this Jul 30, 2021
@gregw gregw added this to In progress in Jetty 9.4.44 FROZEN via automation Jul 30, 2021
@gregw gregw added this to In progress in Jetty 10.0.7/11.0.7 FROZEN via automation Jul 30, 2021
@gregw gregw linked an issue Jul 30, 2021 that may be closed by this pull request
Removed the last flush of an empty buffer as was no path to that code.
@gregw
Copy link
Contributor Author

gregw commented Jul 30, 2021

@lorban @lachlan-roberts In an attempt to get 100% code coverage, I could not find a path to get to the final last emptybuffer write. So I have removed it and I think it is good. But give me extra good review on that deletion.

Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

You should probably add some tests to HttpOutputTest to assert that writing 0 length buffers does work.

Jetty 9.4.44 FROZEN automation moved this from In progress to Review in progress Jul 30, 2021
Jetty 10.0.7/11.0.7 FROZEN automation moved this from In progress to Review in progress Jul 30, 2021
Restored the last flush of an empty buffer as is needed for 0 length empty write.
@gregw gregw requested a review from lorban July 30, 2021 23:02
@gregw gregw marked this pull request as ready for review July 31, 2021 06:21
Jetty 9.4.44 FROZEN automation moved this from Review in progress to Reviewer approved Aug 2, 2021
Jetty 10.0.7/11.0.7 FROZEN automation moved this from Review in progress to Reviewer approved Aug 2, 2021
@gregw gregw merged commit 5c013a5 into jetty-9.4.x Aug 3, 2021
Jetty 9.4.44 FROZEN automation moved this from Reviewer approved to Done Aug 3, 2021
Jetty 10.0.7/11.0.7 FROZEN automation moved this from Reviewer approved to Done Aug 3, 2021
@gregw gregw deleted the jetty-9.4.x-6562-last-bytebuffer branch August 3, 2021 02:30
gregw added a commit that referenced this pull request Aug 3, 2021
Fixes #6562 the last written bytebuffer calculation.
Also fixed an associated issue with unnecessary flush of an empty when last calculation already signalled last.
@gregw gregw mentioned this pull request Aug 4, 2021
gregw added a commit that referenced this pull request Aug 4, 2021
Fixes #6562 the last written bytebuffer calculation.
Also fixed an associated issue with unnecessary flush of an empty when last calculation already signalled last.
gregw added a commit that referenced this pull request Aug 17, 2021
Fix flaky test from #6562
Disable ipv6 test for #6624
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

HttpOutput.write(ByteBuffer buffer)
2 participants