-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Fixed schedule race and task retention with ExecutorScheduler. #2907
Conversation
// note that since we schedule the emission of potentially multiple tasks | ||
// there is no clear way to cancel this schedule from individual tasks | ||
// so even if executor is an ExecutorService, we can't associate the future | ||
// returned by submit() with any particular ScheduledAction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is somewhat confusing to me. The "potentially multiple tasks" is just that we're scheduling the "this" reference over and over again isn't it?
We are scheduling a single "this" one at a time so that it then drains from the queue to ensure sequential execution.
Thus, the scheduling on the Executor
has little to do with any particular Action0
. Is that what you're saying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the scenario: I submit 2 tasks concurrently. The first one will start the schedule with this
. Now let's assume the executor is busy so this
is waiting in the pool's queue. If I cancel the 2 tasks, in theory, there is no need to run this
anymore but to cancel it, one needs very complicated accounting. So in other terms, the best we can do here is to do the emission loop and check if the particular ScheduledAction is unsubscribed or not. The downside is the long retention of such tasks.
3b8d393
to
74e76e2
Compare
I've discovered another retention problem and updated the PR to fix it as well. |
74e76e2
to
491e615
Compare
Thanks for the explanation. |
Fixed schedule race and task retention with ExecutorScheduler.
Is this fixed? I still see the following exception on 1.0.14. Below is the stack from our service's log: ERROR [2015-09-23 20:54:00,640] com.mycompany.mobile.dropwizard.exception.AbstractExceptionMapper: Error handling request: |
@zhiyanshao You seem to have a different problem than what this PR fixes. Either your pool has been shutdown in the middle or you are running with a bounded internal queue that gets overflown. |
@akarnokd , thank you for your reply. Do you know under what circumstances the pool will be shutdown in the middle and how I can tell if I am running with a bounded internal queue? Are these two scenarios by design and can be fixed in my code? |
It appears you are using some Google threadpool-helper classes. Look where the Executor or ExecutorService is configured in your project. |
Fixes a race condition with the timed schedule (first potentially overwriting the result of the untimed schedule in mas) and a scheduled task retention problem due to not tracking those.