pytest_assertrepr_compare unnecessarily calls os.environ.update every time - increased segfault risk due to getenv not thread safe #955
Labels
Comments
sparrowt
added a commit
to sparrowt/syrupy
that referenced
this issue
Feb 20, 2025
See syrupy-project#955 for a detailed explanation of why this can cause thread safety issues resulting in a segfault when another thread calls `getenv`
sparrowt
added a commit
to sparrowt/syrupy
that referenced
this issue
Feb 20, 2025
If neither operand is a `SnapshotAssertion` then the rest of the code is not going to do anything, so bail early rather than unnecessarily setting up `__terminal_color` etc. which can cause issues c.f. syrupy-project#955
3 tasks
|
I've opened #956 to significantly reduce the probability of this occurring, though as noted there the possibility is not entirely gone. Perhaps a more watertight way to fix this is to avoid modifying |
noahnu
added a commit
that referenced
this issue
Feb 20, 2025
* fix: __terminal_color shouldn't modify os.environ unless it has to See #955 for a detailed explanation of why this can cause thread safety issues resulting in a segfault when another thread calls `getenv` * fix: pytest_assertrepr_compare should be a no-op for other types If neither operand is a `SnapshotAssertion` then the rest of the code is not going to do anything, so bail early rather than unnecessarily setting up `__terminal_color` etc. which can cause issues c.f. #955 * chore: run linter --------- Co-authored-by: Noah Ulster <noah.u@roserocket.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
The short version: having
syrupyinstalled appears to greatly increase the chances of a segmentation fault during multi-threaded browser testing using playwright.The long version:
pytest_assertrepr_comparehook always calls__terminal_coloreven though it doesn't need to most of the time (unless both operands areSnapshotAssertion)__terminal_coloralways callsenv_contexteven though it doesn't need to unless--snapshot-no-colorsis enabledenv_contextcallsos.environ.updateos.environis not thread safe in glibc see:live_serverfixture) ends up callinggetenvfrom native code (e.g. during database connection in my case)To reproduce
Because it is a threading race there is no 100% repro, but in my case it happened approx 1 in 10 times during execution of a test suite of 235 playwright browser integration tests.
This is not an open source codebase so I'm unfortunately unable to provide an example project.
However patching syrupy to not call
env_contextand then running this same playwright test suite repeatedly (200 times!) overnight it did not segfault at all - whereas previously it would reliably reproduce within 5-15 runs.Also this was first seen in our CI automation within 1 day after the PR which added syrupy was merged so there is a strong correlation.
Expected behavior
Simply installing syrupy should not cause intermittent segfaults in unrelated code.
Environment
Further details
In order to help with googling for this issue, instead of screenshots here are textual stack traces (both python faulthandler stacks at exit and also gdb backtrace from the segfault core dump)
GDB backtrace from the segfault, showing the
getenvcall which died:Python stack traces at the time of the segfault:
handle_console_messagewhich at line 154 is doingassert msg.type == "verbose" or page.title() == "Expected title"which fits withsyrupyhaving recently executedpytest_assertrepr_comparefor the first equality comparisonStack trace showing
syrupyupdating the environment (as described in the bullet points at the top)The text was updated successfully, but these errors were encountered: