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

Allow TimerSet to safely handle an executor raising RejectedExecutionError #999

Merged

Conversation

bensheldon
Copy link
Contributor

Fixes #889

The TimerSet creates a reactor-like loop to pop scheduled tasks off a queue at their appropriate time. If the task's executor has a fallback policy of :abort, the executor can raise a Concurrent::RejectedExecutionError within the loop and break it. This PR introduces a rescue to handle and discard that exception.

I think a RejectedExecutionError is the only exception that the executor is expected to raise, but this could be turned into rescuing any StandardError (either with a rescue or wrapping further in a SafeTaskExecutor (that introduces even more blocks and locks).

Also, this only rescues the exception within the loop that handles future-scheduled tasks. There is a magic 0.01-seconds in the TimerSet which will cause the task to be executed immediately rather than deferred which I did not wrap:

if (task.initial_delay) <= 0.01
task.executor.post { task.process_task }
else
@queue.push(task)

@bensheldon bensheldon force-pushed the timer_set_rejected_execution branch 3 times, most recently from 4f1a552 to e062c28 Compare May 28, 2023 20:02
Copy link
Contributor

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks. I think we need to investigate whether task.process_task exceptions are logged - and whether they should indeed cause the execution loop to exit, or whether we should log them and continue executing the loop.

@ioquatix ioquatix merged commit 25ccddc into ruby-concurrency:master Nov 14, 2023
12 checks passed
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

Successfully merging this pull request may close these issues.

ScheduledTask triggering ThreadPoolExecutor's fallback_policy: :abort breaks ScheduledTasks
2 participants