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

Clean shutdown procedure is not invoked when the main thread participates in thread pool execution #123

Closed
pjurewicz opened this issue Jun 5, 2024 · 4 comments · Fixed by #124
Assignees
Labels
bug This issue is a bug.

Comments

@pjurewicz
Copy link

pjurewicz commented Jun 5, 2024

Concurrent::ThreadPoolExecutor is initialized with fallback_policy: :caller_runs
This means that the work will be executed in the thread of the caller, instead of being given to another thread in the pool when the queue of waiting work has reached the maximum size (backpressure) and no new threads can be created.
In such case all the exceptions are rescued in the RubyThreadPoolExecutor#run_task.
This means that Aws::Rails::SqsActiveJob#run rescue block is not triggered and clean shutdown procedure is not invoked.

@pjurewicz pjurewicz changed the title Aws::Rails::SqsActiveJob::Interrupt is not handled when the main thread participates in thread pool execution Clean shutdown procedure is when the main thread participates in thread pool execution Jun 5, 2024
@pjurewicz pjurewicz changed the title Clean shutdown procedure is when the main thread participates in thread pool execution Clean shutdown procedure is not invoked when the main thread participates in thread pool execution Jun 5, 2024
@jterapin
Copy link
Contributor

jterapin commented Jun 5, 2024

Thanks for opening the ticket. We will take a look and get back to you!

@jterapin jterapin added the investigating Issue is being investigated and/or work is in progress to resolve the issue. label Jun 5, 2024
@alextwoods alextwoods added the bug This issue is a bug. label Jun 5, 2024
@alextwoods alextwoods self-assigned this Jun 5, 2024
@alextwoods alextwoods removed the investigating Issue is being investigated and/or work is in progress to resolve the issue. label Jun 5, 2024
@alextwoods
Copy link
Contributor

Thanks for submitting this - good catch. I've had a TODO in the code for awhile to fix the use of caller_runs for backpressure. I'll take a look at fixing that.

@mostlyobvious
Copy link

One more issue with main thread acting as an additional worker — it needs additional connection to the pooled resources over chosen thread count. For example with explicitly chosen 10 worker threads, the database connection pool would have to be 10 + 1 in order to not experience ActiveRecord::ConnectionTimeoutError when backpressure limit is reached.

This is not immediately obvious until experiencing it and reading the code.

@alextwoods
Copy link
Contributor

Yeah this is a good point as well. I had always intended to revisit the use of caller_runs. I believe #124 should make the resource usage more explicit as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants