-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[SQS] Improve shutdown capabilities of SqsProvider #12252
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
Conversation
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 15m 7s ⏱️ - 38m 21s Results for commit 15b4253. ± Comparison against base commit 57aeecb. This pull request removes 1087 tests.
♻️ This comment has been updated with latest results. |
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.
LGTM! Good catch!
I think we could replace the threading.Event
with a boolean in this case, as we don't need to pass the reference around and mutate it. But both works!
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.
I've just realized we have an issue with the CloudwatchDispatcher
: if you call LocalStack reset for SQS, we will shutdown the dispatcher in on_before_stop
, but when calling _start_cloudwatch_metrics_reporting
in on_before_start
we do not reset the flags back, so it will stay in shutdown, as we create the dispatcher in the __init__
method of the provider 🤔 and initialize the event/boolean flag in the __init__
method of the dispatcher too.
Bigger than that, we also don't restart the executor itself. A bit outside of the scope of the PR, but we should probably fix that.
@bentsku There are a couple issues with the |
a248e80
to
15b4253
Compare
Motivation
There are still some hanging threads on SQS shutdown due to:
submit
jobs when executor is shutdownChanges
ThreadPoolExecutor
object is closed (causing aRuntimeException
)