Local variable assignment is broken when combined with threads + tracing + closures #74929
Comments
|
The attached script looks innocent, but gives wildly incorrect results on all versions of CPython I've tested. It does two things:
And most of the increments of the variable are lost! This requires two key things I haven't mentioned, but that you wouldn't expect to change anything. First, the thread function closes over the local variable 'x' (though it doesn't touch this variable in any way). And second, the thread function has a Python-level trace function registered (but this trace function is a no-op). Here's what's going on: When you use sys.settrace() to install a Python-level trace function, it installs the C-level trace function "call_trampoline". And then whenever a trace event happens, call_trampoline calls PyFrame_FastToLocalsWithError, then calls the Python-level trace function, then calls PyFrame_LocalsToFast (see: https://github.com/python/cpython/blob/master/Python/sysmodule.c#L384-L395). This save/call/restore sequence is safe if all the variables being saved/restored are only visible to the current thread, which used to be true back in the days when local variables were really local. But it turns out nowadays (since, uh... python 2.1), local variables can be closed over, and thus can visible from multiple threads simultaneously. Which means we get the following sequence of events:
This means that merely installing a Python-level trace function – for example, by running with 'python -m trace' or under pdb – can cause otherwise correct code to give wrong answers, in an incredibly obscure fashion. In real life, I originally ran into this in a substantially more complicated situation involving PyPy and coverage.py and a nonlocal variable that was protected by a mutex, which you can read about here: https://bitbucket.org/pypy/pypy/issues/2591/ CC'ing belopolsky as the interest area for the trace module, Mark Shannon because he seems to enjoy really pathological low-level bugs, and Benjamin and Yury as the "bytecode" interest area. I'm surprised that there's apparently no interest area for, like, "variables should retain the values they are assigned" -- apologies if I've CC'ed anyone incorrectly. |
njsmith
added
3.7 (EOL)
|
A version of the same problem without threads, using generators instead to get the bug deterministically. Prints 1, 1, 1, 1 on CPython and 1, 2, 3, 3 on PyPy; in both cases we would rather expect 1, 2, 3, 4. |
|
(Note: x.py is for Python 2.7; for 3.x, of course, replace |
|
Some thoughts based on discussion with Armin in #pypy: It turns out if you simply delete the LocalsToFast and FastToLocals calls in call_trampoline, then the test suite still passes. I'm pretty sure that pdb relies on this as a way to set local variables, though, so I think this is probably more of a bug in the test suite than anything else. In some ways, it seems like the most attractive fix to this would be to have the Python-level locals() and frame.f_locals return a dict proxy object whose __setitem__ writes any mutations back to both the fast array and the real frame->f_locals object, so that LocalsToFast becomes unnecessary. As Armin points out, though, that would definitely be a semantic change: there may be code out there that relies on locals() returning a dict – technically it doesn't have to return a dict even now, but it almost always does – or that assumes it can mutate the locals() array without that affecting the function. I think this would arguably be a *good* thing; it would make the behavior of locals() and f_locals much less confusing in general. But he's right that it's something we can only consider for 3.7. The only more-compatible version I can think of is: before calling the trace function, do FastToLocals, and also make a "shadow copy" of all the cellvars and freevars. Then after the trace function returns, when copying back from LocalsToFast, check against these shadow copies, and only write back cellvars/freevars that the trace function actually modified (where by modified we mean 'old is not new', just a pointer comparison). Since most functions have no cellvars or freevars, and since we would only need to do this when there's a Python-level trace function active, this shouldn't add much overhead. And it should be compatible enough to backport even to 2.7, I think. |
|
The writeback-only-if-changed approach sounds like a plausible improvement to me. I'd be hesitant to include such a change in 3.5.4 though, since we don't get a second go at that if something breaks unexpectedly. However, I don't think returning a write-through proxy from locals() would be a good idea, since you can still have race conditions between "read-update-writeback" operations that affect the cells directly, as well as with those that use the new write-through proxy. At the moment, you can only get yourself into trouble by way of operations that have access to LocalsToFast, and those are pretty rare now that If folks really want that write-through behaviour (at least for closure variables), 3.7 will let them write it themselves, since closure cells are becoming writeable from pure Python code: >>> def outer():
... x = 1
... def inner():
... return x
... return inner
...
>>> f = outer()
>>> f.__closure__[0].cell_contents
1
>>> f.__closure__[0].cell_contents = 2
>>> f()
2
>>> f.__closure__[0].cell_contents = "hello"
>>> f()
'hello' |
|
It isn't obvious to me whether the write-through proxy idea is a good one on net, but here's the rationale for why it might be. Currently, the user-visible semantics of locals() and f_locals are a bit complicated. AFAIK they aren't documented anywhere outside the CPython and PyPy source (PyPy is careful to match all these details), but let me take a stab at it: The mapping object you get from locals()/f_locals represents the relevant frame's local namespace. For many frames (e.g. module level, class level, anything at the REPL), it literally *is* the local namespace: changes made by executing bytecode are immediately represented in it, and changes made to it are immediately visible to executing bytecode. Except, for function frames, it acts more like a snapshot copy of the local namespace: it shows you the namespace at the moment you call locals(), but then future changes to either the code's namespace or the object don't affect each other. Except except, the snapshot might be automatically updated later to incorporate namespace changes, e.g. if I do 'ns = locals()' and then later on someone accesses the frame's f_locals attribute, then reading that attribute will cause my 'ns' object to be silently updated. But it's still a snapshot; modifications to the mapping aren't visible to the executing frame. Except**3, if you happen to modify the mapping object while you're inside a trace function callback, then *those* modifications are visible to the executing frame. (And also if a function is being traced then as a side-effect this means that now our 'ns' object above does stay constantly up to date.) Except**4, you don't actually have to be inside a trace function callback for your modifications to be visible to the executing frame – all that's necessary is that *some* thread somewhere is currently inside a trace callback (even if it doesn't modify or even look at the locals itself, as e.g. coverage.py doesn't). This causes a lot of confusion [1]. On top of that, we have this bug here. The writeback-only-if-changed idea would make it so that we at least correctly implement the semantics I described in the long paragraph above. But I wonder if maybe we should consider this an opportunity to fix the underlying problem, which is that allowing skew between locals() and the actual execution namespace is this ongoing factory for bugs and despair. Specifically, I'm wondering if we could make the semantics be: "locals() and f_locals return a dict-like object representing the local namespace of the given frame. Modifying this object and modifying the corresponding local variables are equivalent operations in all cases." (So I guess this would mean a proxy object that on reads checks the fast array first and then falls back to the dict, and on writes updates the fast array as well as the dict.)
Sure, but that's just a standard concurrent access problem, no different from any other case where you have two different threads trying to mutate the same local variable or dictionary key at the same time. Everyone who uses threads knows that if you want to do that then you need a mutex, and if you don't use proper locking then it's widely understood how to recognize and debug the resulting failure modes. OTOH, the current situation where modifications to the locals object sometimes affect the namespace, and sometimes not, and sometimes they get overwritten, and sometimes they don't, and it sometimes depends on spooky unrelated things like "is some other thread currently being traced"? That's *way* more confusing that figuring out that there might be a race condition between 'x = 1' and 'locals()["x"] = 2'. Plus, pdb depends on write-through working, and there are lots of frames that don't even use the fast array and already have my proposed semantics. So realistically our choices are either "consistently write-through" or "inconsistently write-through". [1] https://www.google.com/search?q=python+modify+locals&ie=utf-8&oe=utf-8 |
|
To make the behaviour more consistent in 3.7, I'd be more inclined to go in the other direction: make locals() return a truly independent snapshot when used in a function, rather than sharing a single snapshot between all locals() calls. Shared snapshots that may potentially be written back to the frame locals would then be unique to trace functions, rather than being a feature of function level locals() calls in general. |
|
Interesting idea! I'm not sure I fully understand how it would work though. What would you do for the frames that don't use the fast array, and where locals() currently returns the "real" namespace? How are you imagining that the trace function writeback would be implemented? Some sort of thread-local flag saying "we're inside a trace function for frame XX" that causes locals() and f_locals to switch to returning a "real" namespace object? |
|
Sorry, I wasn't clear: I don't see any problem for the cases that don't optimize local variable access, and don't think any of those should change. Instead, I think we should tighten up the formal specification of locals() to better match how it is actually used in practice: https://mail.python.org/pipermail/python-dev/2013-May/125917.html (https://bugs.python.org/issue17960 is the corresponding issue, although I clearly got distracted by other things and never followed up with a patch for the language reference. https://bugs.python.org/issue17546 is another issue lamenting the current underspecification in this area) However, function bodiess are already inherently different from other execution namespaces, and that stems from a particular special case assumption that we don't make anywhere else: we assume that at compile time, the compiler can see all of the names added to the local namespace of a function. That assumption wasn't quite valid in Python 2 (since unqualified exec statements and function level wildcard imports could mess with it), but it's much closer to being true in Python 3. Checking the 3.7 code, the only remaining ways to trigger it are:
So I think an entirely valid way forward here would be to delete LocalsToFast in 3.7+, and say that if you want to write access to a function namespace from outside the function, you need to either implement an eval hook (not just a tracing hook), or else use a closure that closes over all the variables that you want write access to. However, the less drastic way forward would be to make it so that writing a tracing function is the only way to get access to the FastToLocals result, and have locals() on a frame running a code object compiled for fast locals return f->f_locals.copy() rather than a direct reference to the original. |
|
I updated the old "we should clarify the semantics" issue with a more concrete update proposal: https://bugs.python.org/issue17960#msg296880 Essentially nothing would change for module and class scopes, but the proposal for function scopes is that locals() be changed to return "frame.f_locals.copy()" rather than a direct reference to the original. Nothing would change for tracing functions (since they already access frame.f_locals directly), but the current odd side-effect that setting a trace function has on the result of normal locals() calls would go away. Folks that actually *wanted* the old behaviour would then need to do either "sys._getframe().f_locals" or "inspect.currentframe().f_locals". However, none of that would help with *this* issue: resolving the bug here would still require either a modification that allowed PyFrame_LocalsToFast to only write back those values that had been rebound since the preceding call to PyFrame_FastToLocals (ma_version_tag could help with doing that efficiently), or else a decision to disallow write-backs to frame locals even from tracing functions in 3.7+. |
So by making locals() and f_locals have different semantics, we'd be adding yet another user-visible special-case? That seems unfortunate to me.
Disallowing writeback from tracing functions would completely break bdb/pdb, so unless you're planning to rewrite bdb in C as an eval hook, then I don't think this is going to happen :-). Writing back to locals is a useful and important feature! I think I'm missing some rationale here for why you prefer this approach – it seems much more complicated in terms of user-visible semantics, and possibly implementation-wise as well. |
|
I like it because it categorically eliminates the "tracing or not?" global state dependence when it comes to manipulation of the return value of locals() - manipulating that will either always affect the original execution namespace immediately (modules, classes, exec, eval), or always be a completely independent snapshot that can never lead to changes in the original name bindings (functions, generators, coroutines). As a result, the locals() documentation updates for issue bpo-17960 wouldn't need to mention trace functions at all. Instead, the only folks that would need to worry about potentially unexpected updates to the internal state of functions, generators, and coroutines when tracing is in effect would be those accessing frame.f_locals directly. That state dependence can then be documented specifically as part of the f_locals documentation, and users of that attribute can make their own copy if they want to ensure that their subsequent mutations definitely can't affect the original namespace, even when tracing is in effect. |
Maybe I was unclear...? my question is why you prefer locals-making-a-copy over locals-and-f_locals-for-function-frames-return-proxy-objects. It seems to me that both of these proposals eliminate the "tracing or not?" global state dependence (right?), so this can't be a reason to prefer one over the other. And the latter additionally eliminates the distinction between modules/classes/exec/eval and functions/generators/coroutines, while also avoiding introducing a distinction between locals() and f_locals. |
|
The problem I see with proxy objects for functions/coroutines/generators is that it *doesn't* match how locals() currently behaves in the absence of a tracing function - that gives you a "single shared snapshot" behaviour, where writes to the result of locals() *don't* affect the original namespace. I agree that replacing frame.f_locals with a write-through proxy would be a good way to get rid of PyFrame_LocalsToFast, though (and thus fix the bug this issue covers). The point where we disagree is that I think we should replace the tracing-or-not distinction with a locals()-or-frame.f_locals distinction, not get rid of the distinction entirely. |
|
In msg296758 Nathaniel wrote:
Maybe a point to be taken in this issue is that pdb does a poor usage of this feature as shown in bpo-9633: changing the value of a local only works in limited cases. |
|
As per the discussion in issue bpo-17960, I'm going to put together a short PEP about this topic for Python 3.7 that will better specify the expected behaviour of locals() and frame.f_locals. That will be a combination of making the status quo official, proposing some changes, and asking Guido for a design decision. Documenting the status quo: make the current behaviour at module scope, class scope, and the corresponding behaviours of exec and eval officially part of the status quo Proposing a change: replacing the current LocalsToFast approach with a custom write-through proxy type that updates the locals array and nonlocal cell variables immediately whenever frame.f_locals is modified (this is the part that should fix the bug reported in this issue) Requesting a design decision: whether locals() at function(/generator/coroutine) scope should return:
|
|
Err, s/officially part of the status quo/officially part of the language specification/ :) |
|
After drafting PEP-558, briefly chatting about it to Guido, and sleeping on the topic, I'm wondering if there might be answer that's simpler than any of the alternatives consider so far: what if PyFrame_FastToLocals added the *cell objects* to f_locals for any variables that the compiler would access using LOAD/STORE_DEREF (rather than LOAD/STORE_FAST), and then PyFrame_LocalsToFast only wrote back the entries for variables that used LOAD/STORE_FAST? That would be enough to fix the reported problems (since PyFrame_LocalsToFast would always leave closure variables alone in both the function defining the closure and the ones referencing it), and a trace hook that actually *wanted* to update the closure references could write to the "cell_contents" attribute on the cell of interest. |
|
How does a trace function or debugger tell the difference between a closure cell and a fast local whose value happens to be a cell object? |
|
The same way the dis module does: by looking at the names listed in the various code object attributes. If it's listed in co_cellvars, then it's a local variable in the current frame that's in a cell because it's part of the closure for a nested function. If it's listed in co_freevars, then it's a nonlocal closure reference. Otherwise, it's a regular local variable that just happens to be holding a reference to a cell object. So if all we did was to put the cell objects in the frame.f_locals dict, then trace functions that supported setting attributes (including pdb) would need to be updated to be cell aware: def setlocal(frame, name, value):
if name in frame.f_code.co_cellvars or name in frame.f_code.co_freevars:
frame.f_locals[name].cell_contents = value
else:
frame.f_locals[name] = valueHowever, to make this more backwards compatible, we could also make it so that *if* a cell entry was replaced with a different object, then PyFrame_LocalsToFast would write that replacement object back into the cell. Even with this more constrained change to the semantics frame.f_locals at function level, we'd probably still want to keep the old locals() semantics for the builtin itself - that has lots of string formatting and other use cases where having cell objects suddenly start turning up as values would be surprising. |
|
https://github.com/python/cpython/pull/3640/files includes a more automated-tested-friendly version of Armin's deterministic reproducer (the generator loop uses range() to generator a separate loop counter, and then the test checks that the incremented closure variable matches that loop counter), together with a fix based on the idea of putting actual cell objects into frame.f_locals while a trace hook is running. It turned out a lot of the necessary machinery was already there, since CPython already uses a common pair of utility functions (map_to_dict/dict_to_map) to handle fast locals, cell variables, and nonlocal variables. PEP-558 still needs another editing pass before I send it to python-dev, and the PR needs some additional test cases to explicitly cover the expected locals() and frame.f_locals semantics at different scopes when a Python-level trace hook is installed, but I'm happy now that this is a reasonable and minimalistic way to resolve the original problem. |
|
Doesn't this proposal break every debugger, including pdb? |
|
For write-backs: no, since the interpreter will still write those values back into the destination cell For locals display: no, since nothing changes for the handling of fast locals For closure display: yes as, by default, debuggers will now print the closure cell, not the value the cell references - they'd need to be updated to display obj.cell_contents for items listed in co_freevars and co_cellvars. That's why PEP-588 needs to be a PEP - there's a language design question around the trade-off between requiring all future Python implementations to implement a new write-through proxy type solely for this use case, or instead requiring trace functions to cope with co_freevars and co_cellvars being passed through as cell objects, rather than as direct references to their values. |
|
I've been thinking further about the write-through proxy idea, and I think I've come up with a design for one that shouldn't be too hard to implement, while also avoiding all of the problems that we want to avoid. The core of the idea is that the proxy type would just be a wrapper around two dictionaries:
Most operations on the proxy would just be passed through to f_locals, but for keys in both dictionaries, set and delete operations would *also* affect the cell in the cell dictionary. (Fortunately dict views don't expose any mutation methods, or intercepting all changes to the mapping would be a lot trickier) Frames would gain a new lazily initialised "f_traceproxy" field that defaults to NULL/None. For code objects that don't define or reference any cells, nothing would change relative to today. For code objects that *do* define or reference cells though, tracing would change as follows:
This preserves all current behaviour *except* the unwanted one of resetting cells back to their pre-tracehook value after returning from a trace hook:
That way, we can avoid the problem with overwriting cells back to old values, *without* enabling arbitrary writes to function locals from outside trace functions, and without introducing any tricky new state synchronisation problems. |
|
Wow, nothing here is simple. :-( Even though the examples show that there's obviously a bug, I would caution against fixing it rashly without understanding how the current behavior may be useful in other circumstances. I've lost what I recall from reading PEP-558 so I can't quite fathom the new design, but I wish you good luck trying to implement it, and hopefully it will work out as hoped for (simpler, solving the issue, keeping the useful behaviors). |
|
Aye, what's in PEP-558 is the least invasive implementation change I've been able to come up that fixes the reported bug, but Nathaniel's right that it would break debuggers (and probably some other things) as currently written. So the above design comes from asking myself "How can I get the *effect* of PEP-558, while hiding what's going on internally even from trace function implementations?". While I can't be sure until I've actually implemented it successfully (no ETA on that, unfortunately), I think blending my idea with Nathaniel's original idea will let us enjoy the benefits of both approaches, without the downsides of either of them. |
|
I guess I should say that I'm still confused about why we're coming up with such elaborate schemes here, instead of declaring that f_locals and locals() shall return a dict proxy so that from the user's point of view, they Always Just Work The Way Everyone Expects. The arguments against that proposal I'm aware of are:
So the original write-through proxy idea still seems like the best solution to me. |
* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (pythongh-74929) Closes pythongh-11888 (cherry picked from commit 2180991) Co-authored-by: Alyssa Coghlan <[email protected]>
|
Given #118934 covers the issue with Since it's the weekend, I'll leave it open for a few days in case there's something we still need that I've missed :) |
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for pythongh-74929 didn't match either the current behaviour or the intended behaviour once pythongh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once pythongh-118934 is fixed).
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for gh-74929 didn't match either the current behaviour or the intended behaviour once gh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once gh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added)
See Implementation Notes in the updated PEP for details of the correction/clarification and the rationale for it. The discrepancy was detected when writing the documentation for python/cpython#74929 and when reviewing python/cpython#118934
* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (gh-74929) Closes gh-118888 (cherry picked from commit 2180991) Co-authored-by: Alyssa Coghlan <[email protected]>
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for pythongh-74929 didn't match either the current behaviour or the intended behaviour once pythongh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once pythongh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added) (cherry picked from commit fd6cd62) Co-authored-by: Alyssa Coghlan <[email protected]>
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for gh-74929 didn't match either the current behaviour or the intended behaviour once gh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once gh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added) (cherry picked from commit fd6cd62) Co-authored-by: Alyssa Coghlan <[email protected]>
* Add docs for new APIs * Add soft-deprecation notices * Add What's New porting entries * Update comments referencing `PyFrame_LocalsToFast()` to mention the proxy instead * Other related cleanups found when looking for refs to the deprecated APIs
* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (pythongh-74929) Closes pythongh-11888
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for pythongh-74929 didn't match either the current behaviour or the intended behaviour once pythongh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once pythongh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added)
Triage: okay to close now? |
|
Given that python/steering-council#245 proved more complicated than we expected, I think it's better to keep this open for now (this and #118934 will probably end up being closed at the same time). |
* Add docs for new APIs * Add soft-deprecation notices * Add What's New porting entries * Update comments referencing `PyFrame_LocalsToFast()` to mention the proxy instead * Other related cleanups found when looking for refs to the deprecated APIs
* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (pythongh-74929) Closes pythongh-11888
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for pythongh-74929 didn't match either the current behaviour or the intended behaviour once pythongh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once pythongh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added)
* expand on What's New entry for PEP 667 (including porting notes) * define 'optimized scope' as a glossary term * cover comprehensions and generator expressions in locals() docs * review all mentions of "locals" in documentation (updating if needed) * review all mentions of "f_locals" in documentation (updating if needed)
* Add docs for new APIs * Add soft-deprecation notices * Add What's New porting entries * Update comments referencing `PyFrame_LocalsToFast()` to mention the proxy instead * Other related cleanups found when looking for refs to the deprecated APIs
* Clarify impact on default behaviour of exec, eval, etc * Update documentation for changes to PyEval_GetLocals (pythongh-74929) Closes pythongh-11888
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for pythongh-74929 didn't match either the current behaviour or the intended behaviour once pythongh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once pythongh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added)
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
Linked PRs
FrameLocalsProxy#118624The text was updated successfully, but these errors were encountered: