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

Jetty-12 HttpContent javadoc #9080

Closed
gregw opened this issue Dec 22, 2022 · 5 comments · Fixed by #12069
Closed

Jetty-12 HttpContent javadoc #9080

gregw opened this issue Dec 22, 2022 · 5 comments · Fixed by #12069

Comments

@gregw
Copy link
Contributor

gregw commented Dec 22, 2022

Jetty version(s)
12

Enhancement Description

The HttpContent contract is entirely undocumented. IF we are to keep the getByteBuffer design, then it needs to be documented and perhaps renamed (it is not a getter as it makes a new buffer each time).

@gregw
Copy link
Contributor Author

gregw commented Dec 22, 2022

We should also complete discussions on #8790. The work done there needs a formal review so we can record what we do and do not like about it.

@gregw
Copy link
Contributor Author

gregw commented Dec 22, 2022

Note also that it is still entirely unclear to me if getByteBuffer should always return a buffer or not? is it used to load the cache or does the cache choose to directly load buffering HttpContent types using the Resource?
How does an implementation know if it being cached or not? If it is not being cached, then there is no need to duplicate the buffer and perhaps a different buffer type should be used.

@lorban
Copy link
Contributor

lorban commented Jan 9, 2023

A fresh look at HttpContent makes me agree that getByteBuffer has no place there: it's too low-level for the layers where HttpContent is meant to be used.

I feel like a more appropriate API would be something like HttpContent.writeTo(Content.Sink sink) where the content is told to write itself without exposing ByteBuffer, Path or any other low-level detail; "just write yourself down there".

@gregw
Copy link
Contributor Author

gregw commented Jan 10, 2023

@lorban The limitation of HttpContent.writeTo(Content.Sink sink) API is that it does not allow interception of response headers that may affect caching among other things.
@lachlan-roberts work in #8790 does allow header interception, although sometimes you need to wrap in order to wrap in order to intercept, so perhaps still not exactly the best, but at least capable.
@lachlan-roberts can you enumerate the difficult use-case in your branch.. i.e. the ones you don't like the implementation for?

@lachlan-roberts
Copy link
Contributor

I was trying to implement a Processor API using the signature process(Request request, Response response, Callback callback), so the HttpContent could set response headers as well as write the content asynchronously. From memory these were the main issues.

  • Allowing the write to set headers is problematic because the HttpContents are often nested with layers adding their own behaviour, and the inner most HttpContent is the one to write the content, but the outer most HttpContent has the correct headers to set. So this introduced a lot of complexity because we had to intercept the setting of any headers that a wrapper wanted to modify.

  • We needed to be able to write byte ranges which required a duplication of logic between the HttpContent implementations. There were differences between writing from a file and writing from a buffer so there had to be two versions of this logic which were slightly different.

  • To move the Welcome file logic into a HttpContent.Factory it needed to be able to send redirects from the process(...) method. It needed a reference back to the ResourceService because these redirect behaviours are overriden for servlet implementations. Sending errors also needed to be delegated back to the ResourceService.

@lorban lorban moved this to 🏗 In progress in 🧊 Jetty 12.0.12 - FROZEN Jul 24, 2024
@lorban lorban moved this to 🏗 In progress in Jetty 12.1.0 Jul 24, 2024
@lorban lorban linked a pull request Jul 24, 2024 that will close this issue
@lorban lorban closed this as completed Jul 25, 2024
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.1.0 Jul 25, 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 a pull request may close this issue.

5 participants