Closed njsmith closed 6 years ago
Interesting. Honestly, the current Local
implementation probably needs some further review with respect to other Curio features involving threads/processes (run_in_thread()
, run_in_process()
, etc.). I'm not entirely sure it's sound under all use cases. Will add to the list of things to look at.
Pushed a change that addresses the race condition. Will leave open as a reminder to investigate other functions.
You might want to add a __dir__
method as well – the history is a bit confused but afaict that's why the patch thing was added in the first place.
For run_in_*
, I believe the current status is that the local storage just isn't available and attempting to access it will raise an error. For run_in_thread
it's easy to fix this in principle: we could make the task's locals dict accessible and it would be fine because at any given moment only one of (the task, the thread) is running. For run_in_process
things are trickier because there's no guarantee that the locals are pickleable, and anyway we might not want to always copy over all the state on every call, when it's possible it won't even be used. One idea would be to have a constructor flag to pick, like curio.Local(expose_to_subprocesses=True)
.
Hm, I'm not an expert neither in python bytecode hacking nor in GIL, but here is my reasoning. We know that in cpython GIL switches only between bytecode instructions, so an operation is considered atomic in python if it translates to a single bytecode instruction (I take this post as a reference). Furthermore, if we disassemble the current local.Local.__setattr__
:
>>> from curio import local
>>> import dis
>>> dis.dis(local.Local.__setattr__)
120 0 LOAD_FAST 2 (value)
3 LOAD_GLOBAL 0 (_local_dict)
6 LOAD_FAST 0 (self)
9 CALL_FUNCTION 1 (1 positional, 0 keyword pair) # this
12 LOAD_FAST 1 (name)
15 STORE_SUBSCR # and this
16 LOAD_CONST 0 (None)
19 RETURN_VALUE
>>>
we can see that there are two instructions CALL_FUNCTION
and STORE_SUBSCR
that might cause an inconsistency.
I'm not aware of any code in the current implementation that would need a lock around it unless I'm missing something.
Theoretically, someone might be calling
curio.run
simultaneously from two different threads, and theoretically, those two runs might both end up using some code from a single library, and theoretically, that library might contain acurio.Local
. And if all that were true, then theoretically it might happen that the two threads both attempted to access some attribute of thatcurio.Local
object at the same time.If this were to happen, then it's possible that one thread would see the other thread's local values, because currently
curio.Local
mutates the object in-place (see_patch_magic_dict
) and doesn't have any locking.I doubt anyone has hit this in real life, but since I noticed it I figured I'd mention it :-)