Skip to content

Scheduler shutdown capability#3022

Closed
akarnokd wants to merge 2 commits into
ReactiveX:1.xfrom
akarnokd:SchedulerShutdownV2
Closed

Scheduler shutdown capability#3022
akarnokd wants to merge 2 commits into
ReactiveX:1.xfrom
akarnokd:SchedulerShutdownV2

Conversation

@akarnokd

Copy link
Copy Markdown
Member

This comes back from time to time (#1730): some containers, when removing RxJava enabled applications expect it to shut down all its threads but by default, computation threads can't be shut down manually and io threads take 1 minute to shut down on their own.

This PR adds the capability to make them shut down their worker threads more eagerly. Since such shutdown would be terminal and thus break any subsequent test, a restart capability is required.

Therefore, I've introduced the optional SchedulerLifecycle interface which if implemented by a Scheduler, makes it eligible for the Schedulers factory to trigger a shutdown or restart for all kinds of schedulers.

Naturally, this implies some extra cost:

  • The underlying pool of the computation scheduler is no longer constant and involves a volatile get every time a worker is requested. I haven't benchmarked this but it just adds a cheap load on x86 and shouldn't be a performance hit.
  • Since we need to track all ThreadWorkers of the io scheduler, this involves a CompositeSubscription and the cost of starting new workers is increased by the synchronization and HashSet.add() operations. However, since starting a new thread in itself is somewhat expensive, again this shouldn't be a performance hit although it's hard to benchmark it.

In addition, to support proper task rejection after the scheduler has shut down, both scheduler types require a constant shutdown worker. These workers are created at class load time and will spin up a thread for a short duration before they shut it down. They don't affect, performance wise, the normal operations but the app startup time might get increased. The tradeoff here is to save on a mandatory class cast whenever a worker is requested.

@akarnokd

Copy link
Copy Markdown
Member Author

It appears I've made a copy-paste error and missed the RxScheduledExecutorPool.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The javadoc should probably state that this is only needed if restarting after having called shutdown.

Though I'm curious as to why calling start is needed. Why not make it so shutdown resets it back to nothing running, but if anything is invoked it starts up again just like on a fresh JVM start?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make it so shutdown resets it back to nothing running, but if anything is invoked it starts up again just like on a fresh JVM start?

I've started noticing lately that RxComputation threads don't start running until they receive their first task. However, I'm not sure if this behavior is just Java 8u45 or standard. If it isn't standard, replacing the workers with a new set of workers on a shutdown would still leak threads.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't even start the Executor so there is nothing that would start it. We have used that approach in Hystrix for years, and there we used the reset model rather than shutdown/start: https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/Hystrix.java#L42

If we completely shutdown the Executor, and then lazily recreate it, there is nothing to start threads, so it would be safe.

@benjchristensen

Copy link
Copy Markdown
Member

I think this all looks good. @abersnaze I'd appreciate your review of this as well.

My only real question is whether we should have shutdown/start, or just shutdown or reset that shuts everything down and resets it to a state where it will automatically start up again if anything is used without needing an explicit start.

Everything else seems to be internal changes and not public API.

@didot

didot commented Jul 1, 2015

Copy link
Copy Markdown

Cool, I was hoping for this feature!

Small question: would it make sense for the user to be able to control wether the threads execute in daemon mode when the user controls the lifecycle of the threads ? Schedulers.computation() threads are created in daemon mode by default

@benjchristensen

Copy link
Copy Markdown
Member

Holding off as I want to finish discussing the shutdown/start vs reset API decision.

@akarnokd

Copy link
Copy Markdown
Member Author

I'll submit a new PR since Eclipse rebase is broken at the moment.

@artem-zinnatullin

Copy link
Copy Markdown
Contributor

@akarnokd Why not to rebase from the Terminal? I can help you with that :)

Had never trusted IDEs for such Git actions…

@akarnokd akarnokd deleted the SchedulerShutdownV2 branch September 9, 2015 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants