search

HypothesisWorks / hypothesis

Hypothesis is a powerful, flexible, and easy to use library for property-based testing.
https://hypothesis.works
Other
7.39k stars 578 forks source link

fix: Race condition inside internal cache implementation #4013

Closed Stranger6667 closed 1 week ago

Stranger6667 commented 2 weeks ago

The issue is when multiple tests share the same cache kwargs, it may lead to an error like the one reported in https://github.com/schemathesis/schemathesis/issues/2269

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

jobh commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

Yep, the RELEASE.rst file is there :)

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

jobh commented 1 week ago

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!