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

Review HttpClient's ContentProvider #4400

Closed
sbordet opened this issue Dec 4, 2019 · 1 comment · Fixed by #4618
Closed

Review HttpClient's ContentProvider #4400

sbordet opened this issue Dec 4, 2019 · 1 comment · Fixed by #4618
Assignees

Comments

@sbordet
Copy link
Contributor

sbordet commented Dec 4, 2019

Jetty version
10

Description
The initial mistake was to model ContentProvider to return an Iterator<ByteBuffer> for the content.

This does not play well because Iterator does not support asynchronous content (e.g. when the client provides some content but not all of it, and then there is a pause: Iterator is not really the right abstraction for this "paused" content).

Furthermore, the type of Iterator being ByteBuffer is not quite enough as sometimes the content is provided with the pair (ByteBuffer, Callback) - completing the callback will recycle the buffer or otherwise signal to provide more content.
This is typical when implementing proxies: the content arriving to the proxy comes already with an attached callback object of some kind to implement backpressure, and we need a way to feed it to the proxy client to send it upstream, and be notified only when the send upstream completed.

Unfortunately, ContentProvider is an API class that users may implement to provide their own content. During its evolution, the initial design showed its limits and for Jetty 10 is probably best to review the design (perhaps in some reactive fashion).

@sbordet
Copy link
Contributor Author

sbordet commented Dec 4, 2019

See also #3756 for additional comments.

sbordet added a commit that referenced this issue Feb 28, 2020
Introduced Request.Content with a reactive model to provide
request content.
Introduced RequestContentAdapter to wrap ContentProviders
into Request.Content.
Updated implementation to use the reactive model rather than
the old pull model.
Reimplemented all ContentProviders in terms of Request.Content.
Converted most of the tests from ContentProvider to Request.Content.
Updated proxy servlets and documentation.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Mar 24, 2020
Improved javadocs and comment as per initial review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Mar 26, 2020
Review updates.
* Now AbstractRequestContent supports multiple subscriptions.
* Reviewed abort() path to fail the content and the subscription
  and notify FailureListener sequentially with other listeners.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Mar 27, 2020
Review updates.
* Updated AbstractRequestContent (and subclasses) failure handling.
* Updated MultiPartRequestContent failure handling.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Mar 30, 2020
Review updates.
Closing MultiPartRequestContent before sending it.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Mar 30, 2020
…ient_content

Issue #4400 - Review HttpClient's ContentProvider.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant