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

Schedulers arent behaving as documented. #16

Closed
nathanburrell opened this issue Sep 8, 2021 · 4 comments
Closed

Schedulers arent behaving as documented. #16

nathanburrell opened this issue Sep 8, 2021 · 4 comments

Comments

@nathanburrell
Copy link

In the documentation of ReactorCallAdapterFactory it states that if you provide a scheduler it will modify the outcome of the request to be executed on a thread managed by the scheduler provided by using publishOn.

I would assume this functionality is added to provide a way for users to get off the netty response thread quickly after returning from webclient where you may potentially do blocking work on the netty response thread downstream as documented on https://docs.spring.io/spring-framework/docs/current/reference/html/web-reactive.html#webflux-concurrency-model

However in the code it doesnt use publishOn it uses subscribeOn ReactoryCallAdapter which I believe does nothing except moves the subscription to the schedulers thread, than executes the request and afterwards leaves the response to be handled on the netty thread incorrectly.

I believe this should be using publishOn as documented to move the response handling to a thread controlled by the scheduler provided.

I originally reported on a dependant project: spring-projects-experimental/spring-cloud-square#27

@JakeWharton
Copy link
Owner

Retrofit does not use Netty. It uses OkHttp.

The create/createWithScheduler factories will use a blocking HTTP call that runs on the subscription thread with the latter automatically moving that thread to the supplied scheduler. By virtue of being blocking, this also affects the thread on which you observe downstream notifications.

createAsync uses OkHttp's async HTTP call API which is mostly just a wrapper around blocking calls except using OkHttp's internal thread pool and honoring more settings (like number of concurrent calls allowed to a host). The subscription thread is only used to fire the async call and the downstream observation thread will be on OkHttp's internal thread pool.

All of this to say, subscribeOn is correct and I see where the documentation incorrectly states publishOn. I will make the documentation change when I get back from vacation.

@nathanburrell
Copy link
Author

nathanburrell commented Sep 9, 2021

Retrofit does not use Netty. It uses OkHttp.

I know that, but sping-cloud-square uses this library to generate netty/webclient clients using retrofit and suffers from this "bug". I raised this based on the documentation as publishOn would solve that problem described above.

Ill move the bug to their library then as they will need to add support to do this as they use netty/webclient.

@nathanburrell
Copy link
Author

The subscription thread is only used to fire the async call and the downstream observation thread will be on OkHttp's internal thread pool.

Interestingly how does okhttp handle this what if the downstream does some long running operations does that mean it uses okhttps thread holding it up from serving other responses/requests? Or does okhttp maintain a separate thread pool for requests and responses?

@JakeWharton
Copy link
Owner

Yes you will block OkHttp's internal thread pool. There are dedicated threads inside OkHttp to keep things like HTTP/2 connections making forward progress even if you are occupying its request/response threads, but eventually you can exhaust them. The idea is that HTTP request overhead is an order of magnitude or two more than what you tend to do in response to them so it's okay to reuse its thread and keep it busy for a bit. If you are doing something very heavy, we expect you to have your own work queue/thread pool pattern.

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

No branches or pull requests

2 participants