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

Improve ssl buffers handling #8165

Merged
merged 6 commits into from
Jun 15, 2022

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Jun 14, 2022

Solves #8161

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban requested review from gregw, joakime and sbordet June 14, 2022 15:53
@lorban lorban self-assigned this Jun 14, 2022
@lorban lorban added the Sponsored This issue affects a user with a commercial support agreement label Jun 14, 2022
@lorban lorban added this to In progress in Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 via automation Jun 14, 2022
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I don't like the "clear to force a release later" pattern. Instead just have clearly named methods that release if empty vs always releases:

  • releaseEmptyInputBuffers; releaseInputBuffers
  • releaseInputBuffersIfEmpty; clearAndReleaseInputBuffers

But ultimately, we should not need comments like "clearing so a later release will take effect". Instead we should just release immediately in those locations - the subsequent final releases are already null check protected.

Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from In progress to Review in progress Jun 14, 2022
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

I agree with @gregw comments about having a discard*Buffer that does both clearing and releasing.

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban requested review from gregw and sbordet June 15, 2022 07:41
gregw
gregw previously approved these changes Jun 15, 2022
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from Review in progress to Reviewer approved Jun 15, 2022
@sbordet sbordet merged commit 66de7ba into jetty-10.0.x Jun 15, 2022
Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from Reviewer approved to Done Jun 15, 2022
@sbordet sbordet deleted the jetty-10.0.x-8161-ssl-buffers-handling branch June 15, 2022 13:10
@macfarla macfarla mentioned this pull request Jun 22, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sponsored This issue affects a user with a commercial support agreement
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants