fix functhread daemon status and tmp_thread cleanup #10404
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thrau
added
the
semver: minor
dominikschubert
approved these changes
Mar 6, 2024
Member
dominikschubert
left a comment
dominikschubert
left a comment
There was a problem hiding this comment.
LGTM, looks safe to me. Appreciate the new logging, just added minor nits/suggestions to make the wording a bit clearer
Comment on lines
+130
to
+132
| LOG.warning( | ||
| "[shutdown] Non-daemon thread %s may block localstack shutdown", thread | ||
| ) |
| thread.stop(quiet=quiet) | ||
| except Exception as e: | ||
| print(e) | ||
| LOG.debug("[shutdown] Error stopping thread %s: %s", thread, e) |
Member
There was a problem hiding this comment.
might the stack trace be interesting for us as well? 🤔
Member
Author
There was a problem hiding this comment.
given we only had a print statement so far, lets see how far we get :D
| ) | ||
| else: | ||
| threading.Thread.__init__(self) | ||
| threading.Thread.__init__(self, daemon=daemon) |
Member
There was a problem hiding this comment.
Nice catch, definitely clearer that way.
Not sure if that actually caused issues though since using the setter is also part of the official API. Only difference in behavior should be the inheritance of the daemon state when passing None.
Co-authored-by: Dominik Schubert <dominik.schubert91@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
I noticed log lines in the builds during shutdown
After some digging, I found that our new stepfunctions provider is putting plain python threads into
TMP_THREADSthat don't have astopattribute. This PR doesn't fix this problem, but it improves error logging during shutdown for these types of issues.While doing that, I noticed that those stepfunction threads aren't daemon threads, and may potentially be blocking the shutdown, so I added specific logging for that as well.
I also noticed that
FuncThreadsetsself.daemon = True, but it's never passed to the constructor which does some additional setup to create a daemonic thread.Changes
FuncThreadinstances are now properly instantiated as daemon threads