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

fix: Race condition inside internal cache implementation #4013

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

Stranger6667
Copy link
Collaborator

@Stranger6667 Stranger6667 commented Jun 18, 2024

The issue is when multiple tests share the same cache kwargs, it may lead to an error like the one reported in schemathesis/schemathesis#2269

Hopefully, the performance impact is negligible. At the moment I can't come up with a reasonably small test to reproduce the issue.

Signed-off-by: Dmitry Dygalo <dmitry@dygalo.dev>
@jobh
Copy link
Contributor

jobh commented Jun 19, 2024

Thanks for your contribution @Stranger6667!

Be aware that we don't test or formally guarantee the thread safety of Hypothesis tests (https://hypothesis.readthedocs.io/en/latest/details.html#thread-safety-policy).

However, the change looks good to me, and I agree it should not significantly slow down single-thread performance. Is my understanding correct that it effectively makes the cache per-thread? If so, there may be a concern about total memory usage — but again, not in the single threaded case.

To proceed, you will need to create hypothesis-python/RELEASE.rst, there's a sample file in the same folder you can look at. The other failing tests are caused by the recent numpy 2 release, and will be fixed by a rebase once #4011 is merged.

@Stranger6667
Copy link
Collaborator Author

Thank you for checking the PR, @jobh!

As far as I remember, there were some tests I added some years ago for a thread-safety issue in RecursiveStrategy, not sure if the policy has changed since then.

Is my understanding correct that it effectively makes the cache per-thread?
If so, there may be a concern about total memory usage — but again, not in the single threaded case.

Yes, you are right. I went this road mainly due to its simplicity and that there are already some places that use threading.local in a similar way. Depending on the usage pattern, it might be indeed worthwhile to add synchronization instead here, i.e. if it is called rarely, this will trade smaller memory usage for the multi-threaded case for a small performance penalty in the single-threaded case. However, I didn't check this in detail, assuming that the memory consumption caused by per-thread caching will be acceptable.

Yep, the RELEASE.rst file is there :)

@jobh
Copy link
Contributor

jobh commented Jun 19, 2024

Yep, the RELEASE.rst file is there :)

My apologies 😁 I saw the whole-repo failure and thought this was the issue. It is just another numpy issue which is fixed elsewhere.

@jobh
Copy link
Contributor

jobh commented Jun 20, 2024

Thank you for checking the PR, @jobh!

As far as I remember, there were some tests I added some years ago for a thread-safety issue in RecursiveStrategy, not sure if the policy has changed since then.

No, not at all. Just a heads-up, that's all; we're happy if it works!

@Zac-HD Zac-HD closed this Jun 24, 2024
@Zac-HD Zac-HD reopened this Jun 24, 2024
@Zac-HD Zac-HD enabled auto-merge June 24, 2024 04:04
@Zac-HD Zac-HD merged commit d651700 into HypothesisWorks:master Jun 24, 2024
102 of 109 checks passed
@Stranger6667 Stranger6667 deleted the dd/fix-cache-race branch June 24, 2024 05:21
The-Compiler added a commit to qutebrowser/qutebrowser that referenced this pull request Jun 29, 2024
Starting with the upgrade to Hypothesis 6.103.4 we got hangs when pytest exits.
This is caused by:

HypothesisWorks/hypothesis#4013

combined with:

python/cpython#102126

which was fixed in Python 3.10.11, but the latest 3.10 packaged by Archlinux was
3.10.10.

Thus, we instead build a newer 3.10 from the AUR.
This bumps the build time up to about 20 minutes on my machine, which is
probably acceptable since those are nightly builds only anyways. We could
probably half that by disabling --enable-optimization, but that would be at the
cost of making the actual test runs (which run more often) slower.

Closes #8247
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants