Fix several issues uncovered while investigating recent CI failures #155
Conversation
This file contains 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
These variables are unused, as `ProcessManager.create_from_pid` now parses the maps file itself; the caller doesn't need to. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Previously we were pausing the process only after we had already fetched the processes mappings, leading to a potential TOCTOU issue: the fetched mappings might be stale by the time the process was paused and we were ready to use them. Handle this by creating a new `ProcessTracer` class responsible for pausing the process. This class matches the behavior that was previously found in `BlockingProcessMemoryManager`, except that it's not also an instance of `AbstractRemoteMemoryManager`: it handles only tracing the process on construction, and detaching on destruction. In blocking mode, we create a `ProcessTracer` before reading the mappings. Later, we pass it to the `ProcessManager`, which takes over ownership of it, destroying it when we're done analyzing the process and want to detach. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
By having the tracer expose the list of thread IDs that it stopped, the manager can avoid a call to find the list of thread IDs itself. Since every thread must be paused when there is a tracer, the tracer's list of threads must be accurate. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Previously, we were fetching the process's list of threads once, and iterating over that list attaching to each thread. This is racy, though: after we get the list of threads and before we've suspended every thread, it's still possible for new threads to start or old threads to finish. Handle this by fetching the list of threads at least twice. When we encounter an error trying to attach to a thread, don't report it unless the next call to list all of the threads says that thread still belongs to the process we're attaching to (if the thread has disappeared from the list, then the reason we failed to attach to that thread is that it has died). Similarly, if any new threads show up in the second list of threads, we'll try to pause those, repeating this until the set of threads that we've paused contains every thread that the process currently owns. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
This is called from C++ code that doesn't know how to propagate a Python exception if one should occur. Tell Cython not to allow an exception to escape from this function, and to instead print and discard one if it should occur. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Leverage our existing `_try_to_decode_string` function which uses `errors="replace"` for decoding, guaranteeing that log messages won't be lost entirely if they contain non-UTF-8 data. Also, pass the log message around as a C++ string rather than a C string, which is more efficient anyway (since the size is known and doesn't need to be computed). Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
godlygeek
force-pushed
the
thread_fixes
branch
from
56c1659
to
db30c2f
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
==========================================
+ Coverage 89.71% 89.75% +0.03%
==========================================
Files 48 48
Lines 5505 5524 +19
Branches 873 873
==========================================
+ Hits 4939 4958 +19
Misses 566 566
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pablogsal
approved these changes
Nov 22, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes several issues, predominantly around how we handle mappings that are created or destroyed while we're actively trying to attach, or threads that are started and stopped while we're still in the process of attaching.