Skip to content

JettyClientEngine: elide buffering #3418

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.

Sign up for GitHub

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

Jump to bottom
Merged
jamezp merged 1 commit into from
Mar 9, 2023
Merged

JettyClientEngine: elide buffering #3418

jamezp merged 1 commit into from
Mar 9, 2023

Conversation

@stevenschlansker
Copy link
Contributor

@stevenschlansker stevenschlansker commented Feb 9, 2023

As originally implemented, the JettyClientEngine could not extract responses without buffering the entire response body, since the MessageBodyReader is sync only. Now, RESTEasy has an asynchronous Executor available to the engine.

So, instead of buffering the full response, let's push the body reader to the async executor pool and feed it content as it becomes available. This should play well with the upcoming virtual threads support in the JVM. This also allows us to implement proper flow control: instead of buffering the whole response, we tell Jetty when we have consumed its buffers, so it will limit the amount of in-flight data.

As an added bonus, the request content send is restructured so the content is also streamed out without buffering the entire request body in a ByteArrayOutputStream.

In the best case, this should remove an entirely unnecessary byte[] copy of both request and response data.
I have completed some light testing and will continue to run the code over the coming days to make sure there's no obvious problems.

@stevenschlansker stevenschlansker force-pushed the jetty-client-async-executor branch 6 times, most recently from e02b194 to aba979a Compare February 9, 2023 20:40
@jamezp
Copy link
Member

jamezp commented Feb 11, 2023

Just an FYI, this is not being ignored. I've just been away at meetings all week so I likely won't have time to look at this until next week.

@stevenschlansker
Copy link
Contributor Author

stevenschlansker commented Feb 11, 2023

No rush! We haven't observed any problems with the new code yet, but will continue testing.

@stevenschlansker
Copy link
Contributor Author

stevenschlansker commented Feb 22, 2023

@jamezp anything I can do to help this get reviewed? :)

@jamezp
Copy link
Member

jamezp commented Feb 24, 2023

@stevenschlansker Sorry, the last two weeks got away from me. I'm out of the office tomorrow, but I'll try to have a look on Monday.

@wildfly-ci
Copy link
Collaborator

wildfly-ci commented Feb 28, 2023

Hello, stevenschlansker. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

jamezp
jamezp requested changes Mar 7, 2023
View reviewed changes
@stevenschlansker stevenschlansker force-pushed the jetty-client-async-executor branch 2 times, most recently from c22d475 to 84750cc Compare March 7, 2023 17:43
@stevenschlansker
Copy link
Contributor Author

stevenschlansker commented Mar 7, 2023 •
edited
Loading

Ok thanks for the review @jamezp , I have addressed your feedback. The try-with-resources is a bit odd style since I wanted to make sure to catch any exception thrown by close (unlikely). I think the onHeaders callback should work as is, let me know if you have further ideas for how that could be better.

I'll build this and start testing the new version internally. I doubt it'll be any different, but you never know...

@stevenschlansker stevenschlansker force-pushed the jetty-client-async-executor branch 2 times, most recently from 62a44f6 to f41d22a Compare March 7, 2023 18:00
@stevenschlansker stevenschlansker requested a review from jamezp March 7, 2023 18:12
@stevenschlansker stevenschlansker force-pushed the jetty-client-async-executor branch from f41d22a to 50b8cf6 Compare March 7, 2023 18:19
@stevenschlansker
Copy link
Contributor Author

stevenschlansker commented Mar 7, 2023

I don't understand the Javadoc failure, probably not related to this change?

 Error:  Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.8.2:site (default-site) on project resteasy-jaxrs-all: failed to get report for org.apache.maven.plugins:maven-jxr-plugin: Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M3:enforce (ban-bad-dependencies) on project resteasy-client-reactor-netty: Execution ban-bad-dependencies of goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M3:enforce failed: org.apache.maven.shared.dependency.graph.DependencyGraphBuilderException: Could not resolve following dependencies: [org.jboss.resteasy:resteasy-jackson2-provider:jar:6.3.0.Alpha1-SNAPSHOT (test)]: Could not resolve dependencies for project org.jboss.resteasy:resteasy-client-reactor-netty:jar:6.3.0.Alpha1-SNAPSHOT: Could not find artifact org.jboss.resteasy:resteasy-jackson2-provider:jar:6.3.0.Alpha1-SNAPSHOT in jboss-public-repository-group (https://repository.jboss.org/nexus/content/groups/public/) -> [Help 1]

@stevenschlansker stevenschlansker force-pushed the jetty-client-async-executor branch from 50b8cf6 to 8498ad0 Compare March 7, 2023 19:10
@jamezp
Copy link
Member

jamezp commented Mar 8, 2023

The JavaDoc issue isn't related to this PR. It's also an issue on main. The other two timeouts might be. I'm looking into that part now.

@jamezp
Copy link
Member

jamezp commented Mar 8, 2023

@stevenschlansker One last request, If you could squash the commits and prefix the commit message with [RESTEASY-3297], I think this is ready to merge. If you don't have time to deal with that I can absolutely do it.

https://issues.redhat.com/browse/RESTEASY-3297

@stevenschlansker stevenschlansker force-pushed the jetty-client-async-executor branch from 8498ad0 to 591a230 Compare March 8, 2023 19:35
@stevenschlansker
Copy link
Contributor Author

stevenschlansker commented Mar 8, 2023

thanks @jamezp , I reworded and squished the commits.

@jamezp
Copy link
Member

jamezp commented Mar 9, 2023

I think these tests keep timing out because of how some kind of network download slowdown.

@jamezp jamezp merged commit 54fa8f1 into resteasy:main Mar 9, 2023
@stevenschlansker stevenschlansker deleted the jetty-client-async-executor branch March 9, 2023 22:34
@stevenschlansker
Copy link
Contributor Author

stevenschlansker commented Mar 9, 2023

Awesome, thank you!

This was referenced May 3, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jamezp jamezp jamezp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stevenschlansker @jamezp @wildfly-ci