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 Jetty ClientHttpConnector code for buffer handling on error/cancellation [SPR-17424] #21957

Closed
spring-projects-issues opened this issue Oct 23, 2018 · 3 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Oct 23, 2018

Rossen Stoyanchev opened SPR-17424 and commented

The request writing side is using Jetty's ContentChunk callbacks to release the input DataBuffer. However the use of flatMap from writeAndFlushWith could prefetch and requires a Flux#onDiscard hook.


Affects: 5.1.1

This issue is a sub-task of #21941

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 23, 2018

Rossen Stoyanchev commented

Sébastien Deleuze, as far I can see for the response side, at present Jetty simply wraps a byte array along with a NoOp callback to release. That means there isn't much use in us calling succeeded() so couldn't we spare ourselves the copying and simply wrap the byte[] for now? Or if we want to support releasing, we should implement a JettyDataBuffer that works like DefaultDataBuffer and calls the ContentChunk callbacks on release.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 24, 2018

Sébastien Deleuze commented

We are simply wrapping byte array because the Jetty HttpClient uses callback.succeed() to perform 2 things: recycle the buffer and request more content (similar to RS request(1)). The fact that these 2 operations are done at the same time is a blocking issue for us when we want to create a composite buffer for example. They plan to fix this at some point, but not yet done see eclipse/jetty.project#2429.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 24, 2018

Rossen Stoyanchev commented

Okay, so we can't wrap and release later. Thanks for the background.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants