Make classproperty and lazyproperty thread-safe #11224
Conversation
Fixes astropy#11221. A double-checked locking pattern is used to avoid impacting performance in the case where the property is already initialised. For lazyproperty, setter and deleter do take the lock, but I'm assuming those are not as performance-critical as the getter.
Now that I've created the PR I have the PR number.
It was listed under API changes, should be bug fixes.
The lexer and parser generation is all collected together in one place (a new astropy.utils.parsing module), with the side benefit that a lot of code duplication is eliminated. This new shared code uses appropriate locking to ensure that both parser creation and use is thread-safe. Fixes astropy#11225 (when combined with astropy#11224).
|
Thanks for the fast reviews. I have one more question: where should the |
|
I've been thinking about this a bit more: it's probably not necessary to take the lock in the setter and deleter, because if one thread is setting/deleting at the same time as another is accessing the property, the user's code has a race condition anyway. We just need to allow concurrent reads to be safe. |
If the user is calling these concurrently from multiple threads, they already have a race condition in their code (as opposed to using the getter from multiple threads, which they can have a reasonable expectation of working). I did change the deleter to do the deletion atomically, so that as long as no fdel is provided it'll be safe to do concurrent deletion. This should restore performance for classes that use a lazyproperty as a cache and need to invalidate the cache when attributes change (e.g. Time).
I've pushed a change for that. |
I'd added alternate code for deleting, but failed to remove the original.
|
Yes, not guarding the setter/deleter makes sense. As for the location, I'm not sure, maybe @embray has a suggestion... I'll admit that I've simply imported from another test function elsewhere, but it is obvious that eventually that is just going to be a hopeless mess. |
There was a problem hiding this comment.
classproperty is used in io.fits and coordinates too, so maybe @saimn and @adrn want to have a look as well.
As for lazyproperty, it just have to not break existing code until we can drop Python 3.7 and then switch to using cached_property (#9036).
I also wonder if we need a temporary commit to enable running "exotic" architectures and also pyinstaller in this PR (and other PRs on the topic of multi-threading). They are defined in other YAML files inside .github/workflows.
|
|
||
| This makes it easier to provoke race conditions. | ||
| """ | ||
| old = sys.getswitchinterval() |
There was a problem hiding this comment.
Same question as #11226 (comment) . That is, define this in setup_module and teardown_module in this case.
There was a problem hiding this comment.
I don't think it's appropriate to use for the entire module. It will likely slow down the rest of the tests in the module.
I'm also expecting that we'll find more thread safety issues in other modules and write more regression tests, so avoiding duplication of this code seems worthwhile (particularly since I don't yet know what the best value for the magic number is).
|
I've moved the fast_thread_switching fixture to astropy/conftest.py. There is already another fixture in there (ignore_matplotlibrc) so it seemed like a reasonable place for it. |
What is the motivation for that? Note that the thread safety issues I've been fixing are all in Python code, which means that the threads aren't really running in parallel (they're serialised by the GIL), and hence ISA memory models are unlikely to have any impact. |
|
Sounds good to me 👍 |
There was a problem hiding this comment.
The thing I've never liked much about pytest fixtures in conftest is that it becomes quite hard to find out where something is defined. But I don't really have a better suggestion and at least the na me of the fixture does tell what it does... Does anyone else have a better suggestion? If not, I suggest we merge this - it is nice!
There was a problem hiding this comment.
Yeah, conftest.py seems like a good place as any. 👍
As for the exotic arch, it's more of a sanity check but if we don't do it here, we'll find out soon enough when the cron runs, and I know who to ping. 😉
Let's get this in then, so people have plenty of time to test against this in 4.3.dev before feature freeze time.
Thank you, all!
|
@bmerry Thanks for this. |
The lexer and parser generation is all collected together in one place (a new astropy.utils.parsing module), with the side benefit that a lot of code duplication is eliminated. This new shared code uses appropriate locking to ensure that both parser creation and use is thread-safe. Fixes astropy#11225 (when combined with astropy#11224).
The lexer and parser generation is all collected together in one place (a new astropy.utils.parsing module), with the side benefit that a lot of code duplication is eliminated. This new shared code uses appropriate locking to ensure that both parser creation and use is thread-safe. Fixes astropy#11225 (when combined with astropy#11224).
The lexer and parser generation is all collected together in one place (a new astropy.utils.parsing module), with the side benefit that a lot of code duplication is eliminated. This new shared code uses appropriate locking to ensure that both parser creation and use is thread-safe. Fixes astropy#11225 (when combined with astropy#11224).
The lexer and parser generation is all collected together in one place (a new astropy.utils.parsing module), with the side benefit that a lot of code duplication is eliminated. This new shared code uses appropriate locking to ensure that both parser creation and use is thread-safe. Fixes astropy#11225 (when combined with astropy#11224).
The lexer and parser generation is all collected together in one place (a new astropy.utils.parsing module), with the side benefit that a lot of code duplication is eliminated. This new shared code uses appropriate locking to ensure that both parser creation and use is thread-safe. Fixes astropy#11225 (when combined with astropy#11224).
The lexer and parser generation is all collected together in one place (a new astropy.utils.parsing module), with the side benefit that a lot of code duplication is eliminated. This new shared code uses appropriate locking to ensure that both parser creation and use is thread-safe. Fixes astropy#11225 (when combined with astropy#11224).
2.0.0.1 (2021-11-02) ==================== - The underlying universal functions in ``erfa.ufunc`` now work with an ``out`` argument also if the required output is a structured array. [gh-76] 2.0.0 (2021-05-17) ================== - Bundled liberfa version update to v2.0.0. This includes new functionality, and hence pyerfa 2.0.0 cannot run with previous versions of liberfa. - ``erfa.dt_eraLDBODY`` has been corrected to ensure that the 'pv' entry is now of type ``erfa.dt_pv``, so that cross-assignments with that dtype work correctly. [gh-74] - ``erfa_generator`` now also generates a ``test_ufunc.py`` file that runs all the C code tests on the ufuncs, thus verifying the code wrapping worked correctly. As part of that, the ability to give specific output file names has been removed, as it was not used. (Note: these changes have no effect on use of PyERFA.) [gh-71] 1.7.3 (2021-04-25) ================== - Bundled liberfa version update to v1.7.3. - Fixed a bug that caused the output of ``rx``, ``ry``, and ``rz`` to be boolean rather than float for some compilers/OS. [gh-72] 1.7.2 (2021-01-25) ================== - Bundled liberfa version update to v1.7.2. - The classproperty decorator is now thread-safe (backport astropy/astropy#11224).
Description
This pull request is to address thread safety in the decorator utilities.
Fixes #11221.
I've also added unit tests. Testing race conditions is notoriously tricky, so I've made it try 10000 times each. That adds about 1s (for each), which is my attempt at a compromise between not slowing down the tests and trying enough times to trigger the bug. Without the fix it is still not enough to reliably fail, but it should be enough that if someone breaks it in future it will fail sooner or later in CI and prompt someone to look closer.