Reactor-core: Limit the number of threads created by the elastic scheduler

Created on 12 Jul 2019  路  5Comments  路  Source: reactor/reactor-core

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.

statuneed-design typenhancement

Most helpful comment

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.

All 5 comments

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)

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)

~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)~

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.

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

Was this page helpful?
0 / 5 - 0 ratings