The attached script looks innocent, but gives wildly incorrect results on all versions of CPython I've tested.

It does two things:

• spawns a thread which just loops, doing nothing

• in the main thread, repeatedly increments a variable 'x'

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:

• In thread A, a trace event occurs (e.g., executing a line of code)

• In thread A, call_trampoline does PyFrame_FastToLocalsWithError, which saves a snapshot of the current value of 'x'

• In thread A, call_trampoline then starts calling the trace function

• In thread B, we increment 'x'

• In thread A, the trace function returns

• In thread A, call_trampoline then does PyFrame_LocalsToFast, which restores the old value of 'x', overwriting thread B's modifications

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/
It turns out that since this only affects *Python*-level trace functions, merely recording coverage information using coverage.py doesn't normally trigger it, since on CPython coverage.py tries to install an accelerator module that uses a *C*-level trace function. Which is lucky for my particular case. But probably it should be fixed anyway :-).

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.

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 .next() with .__next__(). The result is the same)

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 exec is no longer a statement.

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.)

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.

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:

• via a tracing function (since LocalsToFast gets called after the tracing function runs)
• by injecting an IMPORT_STAR opcode into a function code object (the compiler disallows that in Python 3 and emits a SyntaxWarning for it in Python 2, but the LocalsToFast call is still there in the eval loop)

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+.

Folks that actually *wanted* the old behaviour would then need to do either "sys._getframe().f_locals" or "inspect.currentframe().f_locals".

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.

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 a decision to disallow write-backs to frame locals even from tracing functions in 3.7+.

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.

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).

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:

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.

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:

• a direct reference to the write-through proxy
• a live read-only dict-proxy (ala class __dict__ attributes)
• a point-in-time snapshot (i.e. copying the namespace)

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] = value

However, 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:

• the existing f_locals dictionary
• a new dictionary mapping cell & free variable names to their respective cells (note: this may not actually need to be a dict, as a direct reference from the proxy back to the frame may also suffice. However, I find it easier to think about the design by assuming this will be a lazily initialised dict in its own right)

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:

• before calling the trace function:
  • f_locals would be updated from the fast locals array and current cell values as usual
  • f_locals on the frame would be swapped out for f_traceproxy (creating the latter if needed)
• after returning from the trace function:
  • f_locals on the frame would be reset back to bypassing the proxy (so writes to f_locals stop being written through to cells when the trace hook isn't running)
  • only the actual locals would be written from f_locals back to the fast locals array (cell updates are assumed to have already been handled via the proxy)

This preserves all current behaviour *except* the unwanted one of resetting cells back to their pre-tracehook value after returning from a trace hook:

• code outside trace hooks can't mutate the function level fast locals or cells via locals() or frame.f_locals (since their modifications will be overwritten immediately before the trace function runs), but *can* treat it as a regular namespace otherwise
• code inside trace hooks can mutate function level fast locals and cells just by modifying frame.f_locals
• all code can display the current value of function level fast locals and cells just by displaying locals() or frame.f_locals
• there's still only one f_locals dictionary per frame, it may just have a proxy object intercepting writes to cell variables when a trace hook is running

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:

1. Implementing a full dict-like mapping object in C is tiresome. But your proposal above also requires doing this, so presumably that's not an issue.

2. We want to keep the current super-complicated and confusing locals() semantics, because we like making life difficult for alternative implementations (PyPy at least exactly copies all the weird details of how CPython's locals() works, which is why it inherited this bug), and by making the language more confusing we can encourage the use of linters and boost Python's Stackoverflow stats. ...I guess my bias against this argument is showing :-). But seriously, if we want to discourage writing to locals() then the way to do that is to formally deprecate it, not go out of our way to make it silently unreliable.

3. According to the language spec, all Python implementations have to support locals(), but only some of them have to support frame introspection, f_locals, debugging, and mutation of locals. But... I think this is a place where the language spec is out of touch with reality. I did a quick survey and AFAICT in practice, Python implementations either support *both* locals() and f_locals (CPython, PyPy, Jython, IronPython), or else they support *neither* locals() nor f_locals (MicroPython -- in fact MicroPython defines locals() to unconditionally return an empty dict). We could certainly document that supporting writes through locals() is a quality-of-implementation thing CPython provides, similar to the prompt destruction guarantees provided by refcounting. But I don't think implementing this is much of a burden -- if you have enough introspection metadata to get the list of locals and figure out where their values are stored in memory (which is the absolute minimum to implement locals()), then you probably also have enough metadata to write back to those same locations. Plus debugger support is obviously a priority for any serious full-fledged implementation.

So the original write-through proxy idea still seems like the best solution to me.

Given #118934 covers the issue with PyEval_GetLocals, I think this original issue is done!

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 :)

Given #118934 covers the issue with PyEval_GetLocals, I think this original issue is done!

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 :)

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).

While we still have a cleanup/clarification PR to land in the PEPs repo, this is functionally complete as of 3.13.0rc1 (with #118934 and #120906 both resolved), so marking the issue as completed.