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

Limit the number of threads created by the elastic scheduler #1804

Closed
bsideup opened this issue Jul 12, 2019 · 5 comments
Closed

Limit the number of threads created by the elastic scheduler #1804

bsideup opened this issue Jul 12, 2019 · 5 comments
Labels
status/need-design This needs more in depth design work type/enhancement A general enhancement
Milestone

Comments

@bsideup
Copy link
Contributor

bsideup commented Jul 12, 2019

The current implementation of ElasticScheduler does not limit the number of threads created by it.
Since we recommend using it for blocking APIs, it may lead to a problem when there are too many blocking calls and the scheduler will create 1000s of threads.

To avoid that, we should consider limiting the total number of threads and queue the tasks if all threads are busy.

Things to consider

  • this will be a behaviour change and potentially may break existing code, because if the tasks are scheduled recursively (a task submitted to the elastic scheduler submits a few more tasks to it) and wait for them, and the thread pool size is 100, then 101 task will wait forever because all other threads will be waiting
  • Thread pool's size is always a pain to predict / do right

Alternative

Introduce a new Schedulers.blocking() that will be elastic but limit the amount of threads (should be clearly stated in the docs, so that the users understand the potential of having a deadlock as described in "things to consider").

It could be a nice alternative given the elastic nature of both, and even there is some old code that uses elastic, their usage will not dramatically affect the app because their threads will eventually be terminated.

@simonbasle
Copy link
Member

As discussed off-band, I really favor the alternative where we don't touch the current elastic() and newElastic(...) factories. Introducing a cap to these is bound to induce a number of subtle bugs and potentially locking situation without much of a chance for users to anticipate them.

The underlying implementation can be a simple additional parameter to ElasticScheduler, but the API exposition should be separate.

There will be a need for a documentation that provides a good transition path, so that users accustomed to elastic() can discover and migrate to the new capped flavor.

Two open questions remain:

  • naming of the factories
  • configuration of the default instance (like elastic() => what cap to chose)

@simonbasle simonbasle removed status/need-triage warn/behavior-change Breaking change of publicly advertised behavior labels Aug 30, 2019
@simonbasle simonbasle added this to the 3.3.0.RC1 milestone Aug 30, 2019
@bsideup
Copy link
Contributor Author

bsideup commented Aug 30, 2019

Perhaps we can ask help from our Spring friends who most probably already did a research on the default pool size for async tasks (I am thinking pre-Reactive era)

@simonbasle
Copy link
Member

simonbasle commented Sep 17, 2019

This is ready but to close for being battle-tested before 3.3.0.RELEASE, so I'm scheduling it on 3.3.1.RELEASE (haha)

@simonbasle
Copy link
Member

After further discussion with the team, the benefit of offering this new Scheduler right off the .0 release might override the "risk" of introducing a new implementation late in the pre-release cycle. This is quite a flagship feature, and some projects that would benefit from it are not likely to use it if introduced in a patch release.

So we'll work with several of these projects to evaluate the behavior of boundedElastic() when replacing their use of elastic() with it, and potentially decide to integrate it into 3.3.0.RELEASE after all.

The good news is that the approach taken will allow smooth transitioning from [new]Elastic() to [new]BoundedElastic() without any risk to codebases that stick to elastic, no behavior change to be feared or anything like that.

@bclozel
Copy link
Member

bclozel commented Sep 18, 2019

Tested changes in Spring Framework and Spring Boot, with Schedulers.boundedElastic() replacing a few Schedulers.elastic() instances. Everything is green!

bclozel added a commit to spring-projects/spring-framework that referenced this issue Sep 18, 2019
Prior to this commit, Spring Framework would use `Schedulers.elastic()`
in places where we needed to process blocking tasks in a reactive
environment.

With reactor/reactor-core#1804, a new `Schedulers.boundedElastic()`
scheduler is available and achieves the same goal with added security;
it guarantees that resources are bounded.

This commit uses that new scheduler in the standard websocket client,
since the underlying API is blocking for the connection phase and we
need to schedule that off a web server thread.

Closes gh-23661
See gh-23665
bclozel added a commit to spring-projects/spring-boot that referenced this issue Sep 18, 2019
Prior to this commit, Spring Boot would use `Schedulers.elastic()` when
required to process blocking tasks in a reactive environment.
reactor/reactor-core#1804 introduced a new scheduler,
`Schedulers.boundedElastic()` that behaves quite similarly but:

* will limit the number of workers thread
* will queue tasks if no worker thread is available and reject them is
the queue is exceeds a limit

This allows Spring Boot to schedule blocking tasks as before and allows
greater flexibility.

Fixes gh-18269
See gh-18276
pull bot pushed a commit to scope-demo/spring-boot that referenced this issue Sep 18, 2019
Prior to this commit, Spring Boot would use `Schedulers.elastic()` when
required to process blocking tasks in a reactive environment.
reactor/reactor-core#1804 introduced a new scheduler,
`Schedulers.boundedElastic()` that behaves quite similarly but:

* will limit the number of workers thread
* will queue tasks if no worker thread is available and reject them is
the queue is exceeds a limit

This allows Spring Boot to schedule blocking tasks as before and allows
greater flexibility.

Fixes spring-projectsgh-18269
See spring-projectsgh-18276
simonbasle added a commit that referenced this issue Sep 20, 2019
While introducing the singleton version of BoundedElasticScheduler,
we might as well make it truly bounded, including in the number of task
submissions it can "absorb" during a spike. Used to be unbounded, now
bounded to 100K tasks.

This is also more likely to uncover problematic scheduling of blocking
tasks. Users switching from `elastic()` to `boundedElastic()` and
seeing `RejectedExecutionException`s should challenge why their Reactor
app schedules so many blocking tasks, and if still legitimate and
necessary, use a `newBoundedElastic` instance that is better tuned to
their workload.

Reviewed-in: #1900
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/need-design This needs more in depth design work type/enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants