Local variable assignment is broken when combined with threads + tracing + closures

[njsmith]

BPO	30744
Nosy	@arigo, @ncoghlan, @abalkin, @benjaminp, @njsmith, @xdegaye, @markshannon, @1st1, @xgid
PRs	[PEP 558 - WIP] bpo-30744: Trace hooks no longer reset closure state #3640 bpo-35134: Migrate frameobject.h contents to cpython/frameobject.h #18052
Dependencies	bpo-17960: Clarify the required behaviour of locals()
Files	thread-closure-bug-demo.py x.py

^{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:

assignee = 'https://github.com/ncoghlan'
closed_at = None
created_at = <Date 2017-06-24.04:05:28.903>
labels = ['interpreter-core', '3.10']
title = 'Local variable assignment is broken when combined with threads + tracing + closures'
updated_at = <Date 2021-01-29.16:12:19.866>
user = 'https://github.com/njsmith'

bugs.python.org fields:

activity = <Date 2021-01-29.16:12:19.866>
actor = 'terry.reedy'
assignee = 'ncoghlan'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2017-06-24.04:05:28.903>
creator = 'njs'
dependencies = ['17960']
files = ['46971', '46972']
hgrepos = []
issue_num = 30744
keywords = ['patch']
message_count = 48.0
messages = ['296751', '296756', '296757', '296758', '296770', '296806', '296808', '296810', '296841', '296882', '297145', '297150', '297254', '297259', '297480', '297503', '297504', '301769', '301786', '301856', '302420', '302475', '302505', '304099', '304297', '304307', '304315', '304317', '304319', '304323', '304325', '304329', '304345', '304371', '304377', '304381', '304388', '304398', '304399', '304414', '304415', '304416', '304419', '304730', '304745', '304753', '304754', '305586']
nosy_count = 10.0
nosy_names = ['arigo', 'ncoghlan', 'belopolsky', 'benjamin.peterson', 'ionelmc', 'njs', 'xdegaye', 'Mark.Shannon', 'yselivanov', 'xgdomingo']
pr_nums = ['3640', '18052']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue30744'
versions = ['Python 3.10']

Linked PRs

• gh-74929: Implement PEP 667 #115153
• gh-74929: locals() documentation update for PEP 667 #118265
• gh-74929: Rudimentary docs for PEP 667 #118581
• gh-74929: Fix an extra DECREF for PEP 667 implementation #118583
• gh-74929: Make containment checks more efficient in FrameLocalsProxy #118624
• gh-74929: PEP 667 general docs update #119201
• [3.13] gh-74929: PEP 667 general docs update (gh-119201) #119291
• gh-74929: PEP 667 C API documentation #119379
• [3.13] gh-74929: PEP 667 C API documentation (gh-119379) #119892

[njsmith]

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.

[arigo]

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.

[arigo]

(Note: x.py is for Python 2.7; for 3.x, of course, replace .next() with .__next__(). The result is the same)

[njsmith]

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.

[ncoghlan]

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'

[njsmith]

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

[ncoghlan]

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.

[njsmith]

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?

[ncoghlan]

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.

[ncoghlan]

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

[njsmith]

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.

[ncoghlan]

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.

[njsmith]

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.