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

Make succeeded and failed in ICB final + introduce onSuccess #11950

Merged
merged 9 commits into from
Jun 27, 2024

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Jun 24, 2024

Closes #11932

@lorban
Copy link
Contributor Author

lorban commented Jun 25, 2024

@gregw I went over all our ICB implementations that override succeeded and failed, and I came up with two conclusions:

  • Overriding succeeded is often used to release a successfully written buffer before moving on to the next one. But that can easily be replaced by a new onSuccess protected method that can do the same job but in a safe way.
  • Overriding failed is sometimes done to release a buffer, but that can be done in onCompleteFailure instead.
  • There are two places where succeeded and failed are overridden for different reasons, and IMHO those are pure evil. See CustomTransportTest and H3's MessageFlusher to see what I mean: the 1st one calls iterate() from succeeded and does not call super, the 2nd one calls super.succeeded() from failed().

I'm now looking for a way to modify MessageFlusher so that it doesn't need to act this weird, but I'm building a firm belief that overriding succeeded and failed is never, ever a good idea.

@lorban lorban marked this pull request as ready for review June 25, 2024 14:06
@lorban lorban requested a review from gregw June 25, 2024 14:06
@lorban lorban self-assigned this Jun 25, 2024
@lorban
Copy link
Contributor Author

lorban commented Jun 25, 2024

I refactored MessageFlusher and make ICB's succeeded and failed methods final.

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.

Nice big step in the right direction! But a few things....

@lorban
Copy link
Contributor Author

lorban commented Jun 26, 2024

@olamy do you have an idea why the integration tests are failing with a compilation error despite the fact that the code doesn't match what's reported?

@olamy olamy force-pushed the experiment/jetty-12.0.x/11932-icb-subclasses branch 2 times, most recently from a300420 to cc42c3d Compare June 27, 2024 01:02
lorban and others added 9 commits June 27, 2024 11:46
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
…inal

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban force-pushed the experiment/jetty-12.0.x/11932-icb-subclasses branch from 86d79a7 to c0af0e6 Compare June 27, 2024 09:54
@lorban
Copy link
Contributor Author

lorban commented Jun 27, 2024

@gregw I reverted the final modifiers in ICB, and I also fixed a small bug in the delayed onSuccess call and added a test.

I think this can now be merged.

@lorban
Copy link
Contributor Author

lorban commented Jun 27, 2024

@olamy thanks for your help fixing the build!

@joakime joakime merged commit f89e3b8 into jetty-12.0.x Jun 27, 2024
10 checks passed
@joakime joakime deleted the experiment/jetty-12.0.x/11932-icb-subclasses branch June 27, 2024 17:00
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
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Review HttpSender.ContentSender (and other ICB) to remove overridden succeeded method
4 participants