Repository navigation
BUG: ensure thread safety in set_temp_(cache|config) context managers - #19559
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
neutrinoceros
changed the title
set_temp_(cache|config)\ context managersset_temp_(cache|config) context managers
ebe3d88 to
47a2295
Compare
47a2295 to
a4b4f53
Compare
|
Interesting. It looks like I deadlocked the double-test job. Let's see if I can fix that quickly. |
|
indeed I can easily reproduce the deadlock locally. It's hit when my new tests are run for the second time. Interestingly, running just these tests multiple times in a single session doesn't seem to be problematic, so what seems most likely is that some other test is polluting the global state somehow. I don't have a clear idea what to look for just yet but that's what I'll need to figure out next. |
|
another note while I'm thinking about this problem: locking the classes isn't sufficient to provide complete safety; if two threads set a temporary cache to the same location concurrently, the second thread will inherit the first's cache. This is generally to be avoided as it creates a clear vector of attack, though I need to ponder a bit more about how to address it. I'm considering the only thing to do might be to document this as an unsafe pattern, and promote the use of owned uuids to set temporary directories. |
completes nicely so does but halts (deadlock) and so does |
|
In fact, I can deadlock with just which means these test modules just happen to pass normal jobs because their names are alphabetical sorted in the only order that doesn't cause a deadlock. |
|
|
|
Problem found: I forgot to release the lock when an exception was raised. I expect clean CI this time. |
b9b92b6 to
8184cc2
Compare
9a7d2a8 to
9d68832
Compare
9d68832 to
352eb6f
Compare
352eb6f to
684a6de
Compare
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
This is looking good though I have a few comments below, after which we can merge
| @@ -134,11 +140,17 @@ def __init__( | |||
| self._prev_path = self.__class__._temp_path | |||
There was a problem hiding this comment.
This line should probably be in __enter__ after the acquire, otherwise if a thread reaches here while another has already executed __enter__, self._prev_path will get overridden and set to self._path?
| _cfgobjs.clear() | ||
| return super().__enter__() | ||
| except Exception: | ||
| _PATHS_MUTEX.release() |
There was a problem hiding this comment.
We should probably restore _cfgobjs at this point, otherwise if an exception happens during super().__enter__() _cfgobjs will be left empty?
| @@ -0,0 +1,2 @@ | |||
| Ensure thread safety in ``set_temp_cache`` and ``set_temp_config`` | |||
| context managers | |||
There was a problem hiding this comment.
Maybe worth mentioning that concurrency is now explicitly disabled inside these contexts?
| self.__class__._temp_path = self._path | ||
| _PATHS_MUTEX.acquire() | ||
| try: | ||
| self.__class__._temp_path = self._path |
There was a problem hiding this comment.
Is it possible for this to raise an exception? Can we at least combine this try...except with the next one?
There was a problem hiding this comment.
it's indeed very unlikely to raise. The only failure mode I see is if self._path isn't defined, which is something linters and type-checkers should already be warning about. Maybe I over-corrected after my first attempt didn't have enough release hooks, but I agree this one can be dropped.
| # in any order. Hence they need to share a single lock (otherwise deadlocks would be possible). | ||
| # In particular, the possibility of nesting more than one instance of each context imposes | ||
| # the use of a re-entrant lock (RLock). | ||
| _PATHS_MUTEX = RLock() |
There was a problem hiding this comment.
What about naming it _PATHS_LOCK?
There was a problem hiding this comment.
sure. I just like "mutex" a bit more, just because it sounds slightly more ominous to me, but really, not good enough of a reason to break the convention
684a6de to
0ee7b1a
Compare
|
thanks a lot @astrofrog, I baked all your suggestions into a rebased commit ! |
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
Looks good, thanks! One small remaining minor potential issue below, but I'll leave it up to you as to whether to ignore.
| _cfgobjs.clear() | ||
| return super().__enter__() | ||
| except Exception: | ||
| _cfgobjs.update(self._cfgobjs_copy) |
There was a problem hiding this comment.
Unlikely to happen in practice but if _cfgobjs.copy() was to ever raise an exception, self._cfgobjs_copy would be unassigned. One way around this would be to set self._cfgobjs_copy = None and checking that it isn't None here. This is just minor as unlikely, so I'll leave it up to you.
There was a problem hiding this comment.
good point. I'm going to force push again, but for ease of review, the concrete patch I'm pushing is
diff --git a/astropy/config/paths.py b/astropy/config/paths.py
index 06054e1e6d..1e313fb3c5 100644
--- a/astropy/config/paths.py
+++ b/astropy/config/paths.py
@@ -311,7 +311,14 @@ class set_temp_config(_SetTempPath):
_PATHS_LOCK.acquire()
try:
- self._cfgobjs_copy = _cfgobjs.copy()
+ _cfgobjs_copy = _cfgobjs.copy()
+ except Exception:
+ _PATHS_LOCK.release()
+ raise
+
+ self._cfgobjs_copy = _cfgobjs_copy
+
+ try:
_cfgobjs.clear()
return super().__enter__()
except Exception:
0ee7b1a to
ff26994
Compare
2 participants
Description
While working on #18964, I realized that these context managers where obviously not thread safe as they relying on mutations to global state.
Because they are public API, I don't think it's possible to put the genie back in the bottle now and so we must compose with this global+mutable state, and the only thing to do is hence to add locking to it.
Furthermore, because we have to allow any number of threads to call any combination of these context managers in any order, they need to share a single lock (otherwise deadlocks would be possible). In particular, the possibility of nesting more than one instance of each context imposes the use of a re-entrant lock (
RLock).I've added multiple tests that all fail seemingly 100% of the time on my machine so I'm confident they capture the bugs and don't think we need special jobs to run them as stress tests.