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

Implement HttpContent.writeTo() async API #12020

Merged
merged 26 commits into from
Jul 24, 2024

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Jul 9, 2024

Implementation of the HttpContent.writeTo() async API.

Fixes #8790

@lorban lorban changed the base branch from jetty-12.0.x to jetty-12.1.x July 9, 2024 13:59
@lorban
Copy link
Contributor Author

lorban commented Jul 12, 2024

@gregw There are two things I take from you previous comment:

  • org.eclipse.jetty.server.Components should expose getSizedByteBufferPool() instead of getByteBufferPool() as its implementations should always have a default size. IOResources too should be modified to take a sized buffer pool where it expects the pool + size + directness triplet. This refactoring should be done independently in another PR.

  • Making HttpContent.writeTo() take a buffer pool as one of its arguments looks terrible from an API perspective: there is no fundamental reason why writing would require a buffer pool, and most implementations don't: only ResourceHttpContent needs one.
    In my thinking, the sink fundamentally is linked to the channel that does hold a sized pool so we should just expose that by making writeTo() take a subinterface of Content.Sink. This is what I did and it doesn't look too bad, and definitely better than writeTo() taking a sized pool which I tried too.

@gregw
Copy link
Contributor

gregw commented Jul 16, 2024

@lorban I have created #12046 that improves the use of Sized ByteBufferPools.

@lorban
Copy link
Contributor Author

lorban commented Jul 16, 2024

@gregw I like #12046 but it's not solving where should ResourceHttpContent.writeTo() should find its buffer pool.

Let me try something...

@lorban lorban requested a review from gregw July 16, 2024 15:01
@lorban
Copy link
Contributor Author

lorban commented Jul 16, 2024

Here is a summary of the status of the HttpContent overhaul:

in progress:

  • add sized ByteBufferPools in Components (Improve the usage of Sized ByteBufferPool #12046)
  • add HttpContent.writeTo() (the core of this PR)
  • no ByteBufferPool in the signature of HttpContent.writeTo() as only ResourceHttpContent needs it (another proposal in this PR)

still needed:

  • remove HttpContent.release() (only needed by CachingHttpContentFactory, it's too complex and should be rewritten)
  • javadoc HttpContent

optional:

  • Replace ByteBufferPool with sized ByteBufferPool everywhere a pool + size + directness is used (e.g.: IOResources)

Signed-off-by: Ludovic Orban <[email protected]>
@gregw
Copy link
Contributor

gregw commented Jul 17, 2024

@gregw I like #12046 but it's not solving where should ResourceHttpContent.writeTo() should find its buffer pool.

@lorban if you want to suck #12046 into this PR and close it, I'm OK with that.

 - give up on HttpContent.writeTo() overloading
 - standardize offset and length default values

Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
@lorban
Copy link
Contributor Author

lorban commented Jul 17, 2024

@lorban if you want to suck #12046 into this PR and close it, I'm OK with that.

I prefer to focus solely on HttpContent.writeTo() here as I fear merging the two would turn this PR into an omnibus one. Those two concerns can be worked on separately and in parallel so I've reverted any related changes from this PR.

@lorban
Copy link
Contributor Author

lorban commented Jul 18, 2024

Latest update: after a conversation with @sbordet, he managed to convince me that the HttpConfiguration's outputBufferSize and useOutputDirectByteBuffers are unrelated to the buffer size/directness the ResourceHttpContent should use.

ResourceHttpContent is the only HttpContent implementation that requires a sized buffer pool to use in writeTo(), other HttpContent implementations use buffers with freely decided size/directness.

So I've modified ResourceHttpContentFactory so that it takes a sized buffer pool that can then be used by ResourceHttpContent, the buffer size and directness being configured by the instantiators of ResourceHttpContentFactory, i.e.: ResourceServlet or ResourceService which allow configuring those parameters via some servlet init parameters and setters respectively.

So the final signature if the new async API finally has this elegant form:

void writeTo(Content.Sink sink, long offset, long length, Callback callback);

@lorban lorban changed the title 12.1 HttpContent writeTo Implement HttpContent.writeTo() async API Jul 18, 2024
@gregw
Copy link
Contributor

gregw commented Jul 18, 2024

I'm not entirely convinced that the directness is unrelated. But am happy to have an elegant API and use configuration to align sizes and directness.

Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
@lorban
Copy link
Contributor Author

lorban commented Jul 22, 2024

Followup PRs:

Once all those have been merged, the HttpContent rework can be considered done.

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.

Very close now....

@lorban lorban requested a review from gregw July 23, 2024 10:18
@lorban lorban merged commit 834db77 into jetty-12.1.x Jul 24, 2024
5 of 10 checks passed
@lorban lorban deleted the experiment/jetty-12.1.x/HttpContent-writeTo branch July 24, 2024 12:07
@lorban lorban linked an issue Jul 24, 2024 that may be closed by this pull request
@olamy olamy mentioned this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Jetty-12 HttpContent should have an async API
3 participants