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

[Terry J. Reedy]

Change by Terry J. Reedy :


--
versions: +Python 3.10 -Python 2.7, Python 3.5, Python 3.6, Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ionel Cristian Mărieș]

Change by Ionel Cristian Mărieș :


--
nosy: +ionelmc

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Change by Nick Coghlan :


--
pull_requests: +17447
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/18052

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

Starting to make some progress on an implementation, and it occurs to me that 
if this approach does work out, it should make Python level trace functions 
*much* faster.

Right now, the call to the Python function in call_trampoline is bracketed by 
PyFrame_FastToLocals() and PyFrame_LocalsToFast(), even if the trace function 
never accesses frame.f_locals.

By contrast, with the proposed design, PyFrame_LocalsToFast() never gets called 
anywhere (I've actually replaced the entire body with a PyErr_SetString call), 
and PyFrame_FastTo_Locals will only be called in the frame.f_locals descriptor 
implementation.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

When I rejected that approach previously, it hadn't occurred to me yet that the 
write-through proxy could write through to both the frame state *and* the 
regular dynamic snapshot returned by locals().

The current design came from asking myself "What if the proxied reads always 
came from the snapshot, just like they do now, but writes went to *both* 
places?".

So far I haven't found any fundamental problems with the approach, but I also 
haven't implemented it yet - I've only read through all the places in the code 
where I think I'm going to have to make changes in order to get it to work.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

No, as locals() never returns the write-through proxy in the latest version of 
the PEP - it only ever returns the dynamic snapshot namespace (retrieving it 
from the proxy if necessary).

To get access to the write-through proxy in Python level code, you have to 
access frame.f_locals directly (which trace functions will do by necessity, 
since that's the only way they have to get at the function locals).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Xavier de Gaye]

Xavier de Gaye  added the comment:

Nick, in msg304388 you wrote "[allow] immediate write-through from trace 
functions". The latest iteration of PEP 558 says instead:

"tracing mode: the way the interpreter behaves when a trace hook has been 
registered..."

and also says:

"As long as the process remains in tracing mode, then __setitem__ and 
__delitem__ operations on the proxy will affect not only the dynamic snapshot, 
but also the corresponding fast local or cell reference on the underlying 
frame."

So write-through is allowed by the current PEP outside a trace function. Does 
not this change in some cases the behavior of code that updates the mapping 
returned by locals() when tracing mode is active from its standard behavior 
when there is no tracing ?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

I rewrote the PEP based on the latest iteration of the design concept: 
https://github.com/python/peps/commit/9a8e590a523bdeed0598084a0782fb07fc15dfc0

(That initial commit has some minor errors in it that I've already fixed, so 
check the latest version in the repo if you spot anything else - if I'd thought 
it through properly, I would have submitted the update as a PR, so folks had a 
place to more easily make line-by-line comments)

I genuinely like this version, and I think it should be reasonably 
straightforward to implement (given that types.MappingProxyType already exists).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

Just confirming that the locals() vs frame.f_locals distinction also exists in 
the C API: the former is PyEval_GetLocals() (which implicitly ensures the 
snapshot is up to date), while the equivalent of the latter is direct access to 
frame->f_locals (and it's left to code accessing the field to decide whether it 
needs an up to date snapshot or not).

That means I'll be able to change what we store in frame->f_locals, while 
updating PyEval_GetLocals to check for a _FastLocalsProxy instance and return a 
reference to the underlying snapshot instead.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Guido van Rossum]

Change by Guido van Rossum :


--
nosy:  -gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Guido van Rossum]

Guido van Rossum  added the comment:

Sounds like a plan, I will ignore the ticket then.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

Err, I phrased that last sentence really badly.

I meant that if Guido *hasn't* been following this ticket closely, he's more 
likely to ask for clarification if PEP 558 proposes a semantic change based on 
the discussion here without adequately justifying it in the PEP itself.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

Guido: any proposed semantic changes for CPython will eventually make their way 
through PEP 558 (and potentially a competing PEP if someone else is 
sufficiently keen on the idea of making non-tracing mode behave the same way as 
tracing mode), so ignoring the detailed blow-by-blow on this ticket should be 
fine.

If anything, it will make it easier for you to pick up any relevant details of 
the eventual rationale that the PEP text has missed.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Guido van Rossum]

Guido van Rossum  added the comment:

> Moreover, if f_locals can be modified outside a tracing hook, then we have 
> the same problem in a cross-function way, e.g. if function f1() calls 
> function f2() which does sys._getframe(1).f_locals['foo'] = 42.

That's covered by the rule that says sys._getframe() has undefined results.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Guido van Rossum]

Guido van Rossum  added the comment:

@Armin:
> Guido: you must be tired and forgot that locals() is a regular function :-)  
> The compiler cannot recognize it reliably.

I know, but we occasionally do make exceptions for builtins. In particular 
super() is recognized by the compiler.

@Everyone else:
I need to limit my screen time this weekend, hopefully you can either figure 
this out without my help -- there's no way I can keep up with this discussion. 
Sorry!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

The current pdb misbehaviour when navigating the stack is indeed a valid 
argument in favour of allowing immediate write-through from trace functions for 
regular local variables as well - I don't recall reading that bug report when 
Xavier first linked to it, so thanks for bringing it up again.

Another useful point that bug report highlights is that one of the assumptions 
I'd been making (that typical trace hooks would only need to manipulate the 
frame they were called for) is incorrect: via frame.f_back, the trace hook also 
has access to all of the parent frames, so any delayed write-back based 
approach would need to traverse the whole call stack to be truly robust. At 
that point, granting trace functions access to a write-through proxy as 
f_locals for *every* frame starts looking like a far more attractive option.

This suggests to me that rather than only switching behaviours while the 
traceback function is running, we would instead need to make it so that we 
check for whether or not a trace function is installed for the process whenever 
f_locals is looked up. If it is, and the frame uses fast locals, then f_locals 
will be a _FunctionLocalsProxy instance. Otherwise, it will be a regular 
dictionary.

Making a dynamic decision on whether to return a proxy or not is needed to 
handle the pdb stack traversal use case properly: debuggers are often going to 
be confronted with frames that were created *before* their trace hook was 
installed. Right now, as per the issue Xavier linked, attempted writes back to 
those don't tend to work properly (due to when and where the write-back 
happens), but dynamically upgrading to write-through proxies in the presence of 
a trace hook should help resolve that.

While that's a more pervasive change than what I was considering, I think the 
cited pdb bug provides a strong enough rationale to justify the related risk of 
unintended consequences.

Interestingly, I think it also gives us a way to reduce the degree to which 
installation of a trace hook affects the semantics of locals(): when f_locals 
on a frame is a _FunctionLocalsProxy instance, the locals() builtin can still 
bypass it and return the underlying dict. The difference between this and my 
earlier proposal for two different snapshot namespaces is that there'd still 
only be one snapshot, it's that f_locals would either reference it directly 
(the default state), or else via a write-through proxy (the "tracing mode" 
state).

I think that approach will give us the best possible outcome available:

* for code using locals(), the behaviour in non-tracing mode will be completely 
unchanged, and the semantic differences between tracing and non-tracing mode 
while a function is running will be reduced, since the post-trace-hook 
writeback code will be removed, and locals() itself never return a reference to 
the write-through proxy (only the underlying namespace snapshot). (There will 
still be some semantic differences, since the snapshot will be implicitly 
refreshed in tracing mode, whereas you have to call locals() explicitly to 
refresh it in non-tracing mode)

* for code using frame.f_locals, it will either behave the same as locals() if 
no tracing hook is installed when you retrieve a reference from the frame 
(again, no change from the status quo), *or* it will immediately write through 
to the active locals if a tracing hook *is* installed (replacing the current 
post-trace-hook writeback code)

(Note: if folks want to instead argue for the compatibility break option, 
you're going to have to write your own PEP, as I have zero plans to implement 
that myself, and will oppose it as a gratuitously unnecessary compatibility 
break if it does get proposed. Toggling this behaviour change on "Tracing or 
not?" won't be hard, and it ensures any  unforeseen negative outcomes will only 
impact situations that are currently broken anyway)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith  added the comment:

@Nick:
> We're OK with the idea that installing a trace function might automatically 
> turn off various compiler and interpreter managed optimisations

We're OK with assigning to locals() doing that too.

> What we're aiming to avoid is requiring that implementations make the 
> assertion in "a = 1; locals('a') = 2; assert a == 2" pass at function scope, 
> and if anything, we'd prefer to make it a requirement for that assertion to 
> *fail*.

Why do you want it to fail? Given that it's already guaranteed to pass in some 
scopes (definitely in CPython, I'm not sure if it's in the language spec), and 
that we need to support mutations of f_locals for debugging, then this seems 
like a quixotic requirement. But if we do want it to fail, then we should make 
it fail obviously, e.g. by having locals() return a read-only mapping that 
raises an exception on mutation.

The only rationale I can understand here is the desire to leave options open 
for alternative implementations... but in practice all those alternative 
implementations can either support this just fine (because they have to to 
support pdb), or else they're MicroPython and have already stopped supporting 
locals() entirely so it hardly matters how CPython tweaks the details of the 
semantics.

> Requiring locals to actually *be* a write-through proxy (for all locals, not 
> just cell references) would revert Python's semantics to the state they were 
> in before the "fast locals" concept was first introduced, and we have no 
> intention of going back there.

Why not? I don't think anyone has ever suggested that "fast locals" made 
Python's *semantics* simpler.

@Armin:
> Perhaps more importantly, I am sure that if Python starts supporting random 
> mutation of locals outside tracing hooks, then it would open the door to 
> various hacks that are best not done at all, from a code quality point of 
> view.

I'm not saying that assigning to locals() is great style, but there are totally 
legitimate use cases (debugging), and there's nothing stopping those hacks 
right now. You *can* use tracing hooks, or there's this nonsense: 
https://gist.github.com/njsmith/2347382

Once Python commits to supporting something, then generally it exposes it in a 
pretty straightforward way; it's really unusual to put the feature in a 
non-obvious place and then make the obvious way subtly broken to punish people 
who try to use it.

@Nick
> - when do we update ordinary local variables? (this isn't broken, so we want 
> to avoid changing it)

But as pointed out earlier in this thread, pdb's support for updating ordinary 
local variables *is* currently broken, and would be fixed by using a 
write-through proxy for f_locals: https://bugs.python.org/issue30744#msg297480

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

Yep, my current goal is to see if I can come up with a surgical fix that solves 
the established problem with the bad interaction between cells and trace 
functions without any unintended consequences for either CPython or other 
interpreters.

That means the only behaviours I actually *want* to change are those that are 
pretty clearly quirky at best, and outright bugs at worst:

- registering a trace function can result in closure state being reset 
inappropriately, even when none of the code involved accesses locals() or 
f_locals
- registering a trace function may lead to changes to locals() made outside the 
trace function nevertheless being written back to the actual frame state

Establishing a write-through proxy for cell references is definitely fine - 
allowing shared access to closure state is the whole reason we have cell 
objects in the first place.

The more complex case is with regular locals since:

- they used to be regular dictionaries in 1.x, but one of the early 2.x 
releases deliberately changed their semantics with the introduction of fast 
locals
- people *do* sometimes treat the result of locals() at function scope as a 
regular dictionary, and hence they don't always copy it before mutating it 
and/or returning a reference to it
- f_locals is accessible from outside the running function/generator/coroutine, 
so compilers can't just key off calls to locals() inside the function to decide 
whether or not they can see all changes to local variables
- looking for calls to locals() at compile time is dubious anyway, since the 
builtin may have been aliased under a different name (we do something like that 
for zero-arg super(), but that breaks far more obviously when the use of name 
aliasing prevents the compiler from detecting that you need a __class__ 
reference compiled in)
- trace functions nevertheless still need to be able to write their changes 
back to the function locals in order for debuggers to support value injection

My latest design concept for the trace proxy thus looks like this (I've been 
iterating on design ideas to try to reduce the potential memory impact arising 
from merely installing a trace function):

1. The core proxy behaviour would be akin to wrapping f_locals in 
types.MappingProxyType (and I expect the new proxy will be a subclass of that 
existing type, with the tentative name "_FunctionLocalsProxy")

2. The currently planned differences consist of the following:
- setting and deleting items is supported
- holding a reference back to the originating frame (to allow for lazy 
initialisation of the extra state only if the local variables are actually 
mutated through the proxy)
- when a cell variable is mutated through the proxy, the cell gets added to a 
lazily constructed mapping from names to cells (if it isn't already there), and 
the value in the cell is also modified
- when a local variable is mutated through the proxy, it gets added to a set of 
"pending writebacks"

The post-traceback frame update in the trampoline function would then write 
back only the locals registered in "pending writebacks" (i.e. only those 
changes made through the proxy, *not* any incidental changes made directly to 
the result of locals()), which would allow this change to reduce the potential 
local state manipulation side effects of registering a trace function.

If actual implementation shows that this approach faces some technical hurdle 
that makes it infeasible in practice, then I agree it would make sense for us 
to look at alternatives with higher risks of unwanted side effects. However, 
based on what I learned while writing the first draft of PEP 558, I'm currently 
fairly optimistic I'll be able to make the idea work.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Armin Rigo]

Armin Rigo  added the comment:

Guido: you must be tired and forgot that locals() is a regular function :-)  
The compiler cannot recognize it reliably.  Moreover, if f_locals can be 
modified outside a tracing hook, then we have the same problem in a 
cross-function way, e.g. if function f1() calls function f2() which does 
sys._getframe(1).f_locals['foo'] = 42.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

Change by STINNER Victor :


--
nosy:  -haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Guido van Rossum]

Guido van Rossum  added the comment:

Hm I may just be completely off here, but I thought that compilers could be 
allowed to recognize the use of locals() in a particular function and then 
disable JIT optimizations for that function. (In fact I thought we already had 
a rule like this but I can't find any language about it, but maybe I'm mistaken 
and we only have such an exception for sys._getframe() -- though it's not 
mentioned in the docs for that either.)

I do like Nathaniel's idea of making locals() a write-through proxy (and let 
f_locals the same thing). If this keeps the frame alive, well too bad -- there 
are lots of other things that do this too, e.g. tracebacks.

Or what about a read-only proxy or a plain snapshot? The docs already say that 
it *shouldn't* be written and *may* not write through -- are we concerned that 
a lot of people depend on the actual runtime behavior rather than the 
documented behavior?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

I'll also note there's a simpler reason the namespace object exposed at the 
function level can't just be a write-through proxy for the underlying frame: 
references to frame.f_locals may outlive the frame backing it, at which point 
we really do want it to be a plain dictionary with no special behaviour, just 
as it is for regular execution frames. 

(Think "return locals()" as the last line in a helper function, as well as 
variants like "data = locals(); data.pop('some_key'); return data")

That means that no matter what, we need to snapshot the frame locals the when 
frame.f_locals is requested. The question then becomes:

- when we do we update the contents of cell references? (this is what's buggy 
right now when a trace function is installed)
- when do we update ordinary local variables? (this isn't broken, so we want to 
avoid changing it)

Providing write-through support *just* for cells should thus make it possible 
to fix the buggy interaction between cells and trace function, while minimising 
the risk of any unintended consequences affecting regular function locals.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Armin Rigo]

Armin Rigo  added the comment:

Thanks Nick for the clarification.  Yes, that's what I meant: supporting such 
code in simple JITs is a nightmare.  Perhaps more importantly, I am sure that 
if Python starts supporting random mutation of locals outside tracing hooks, 
then it would open the door to various hacks that are best not done at all, 
from a code quality point of view.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

We're OK with the idea that installing a trace function might automatically 
turn off various compiler and interpreter managed optimisations (it's similar 
to how requesting or implying reliance on full frame support in other 
implementations can disable various optimisations). For trace hooks like 
coverage tools and debuggers, that's often downright desirable, since it makes 
the runtime behaviour correlate more directly with the source code.

What we're aiming to avoid is requiring that implementations make the assertion 
in "a = 1; locals('a') = 2; assert a == 2" pass at function scope, and if 
anything, we'd prefer to make it a requirement for that assertion to *fail*.

Requiring locals to actually *be* a write-through proxy (for all locals, not 
just cell references) would revert Python's semantics to the state they were in 
before the "fast locals" concept was first introduced, and we have no intention 
of going back there.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith  added the comment:

@arigo: But CPython is already committed to supporting writes to locals() at 
any moment, because at any moment you can set a trace function and in every 
proposal trace functions can reliably write to locals. So I don't think this is 
a serious obstacle for addng a JIT to CPython -- or at least, it doesn't add 
any new obstacles.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Armin Rigo]

Armin Rigo  added the comment:

FWIW, a Psyco-level JIT compiler can support reads from locals() or f_locals, 
but writes are harder.  The need to support writes would likely become another 
hard step on the way towards adding some simple JIT support to CPython in the 
future, should you decide you ever want to go that way.  (It is not a problem 
for PyPy but PyPy is not a simple JIT.)  Well, I guess CPython is not ever 
going down that path anyway.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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 local
 s()), 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Guido van Rossum]

Guido van Rossum  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Guido van Rossum]

Change by Guido van Rossum :


--
nosy: +gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

Doesn't this proposal break every debugger, including pdb?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Changes by Nick Coghlan :


--
dependencies: +Clarify the required behaviour of locals()

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--
stage: patch review -> 

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Changes by Nick Coghlan :


--
keywords: +patch
pull_requests: +3634
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

Err, s/officially part of the status quo/officially part of the language 
specification/ :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

As per the discussion in issue #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)

--
assignee:  -> ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Xavier de Gaye]

Xavier de Gaye added the comment:

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 issue 9633: changing the value of a local only works in 
limited cases.

--
nosy: +xdegaye

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

Changes by STINNER Victor :


--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

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=utf-8=utf-8

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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'
```

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Xavier G. Domingo]

Changes by Xavier G. Domingo :


--
nosy: +xgdomingo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Armin Rigo]

Armin Rigo added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Armin Rigo]

Armin Rigo added the comment:

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.

--
Added file: http://bugs.python.org/file46972/x.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Armin Rigo]

Changes by Armin Rigo :


--
nosy: +arigo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Changes by Nick Coghlan :


--
nosy: +ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

New submission from Nathaniel Smith:

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.

--
components: Interpreter Core
files: thread-closure-bug-demo.py
messages: 296751
nosy: Mark.Shannon, belopolsky, benjamin.peterson, njs, yselivanov
priority: normal
severity: normal
status: open
title: Local variable assignment is broken when combined with threads + tracing 
+ closures
versions: Python 2.7, Python 3.5, Python 3.6, Python 3.7
Added file: http://bugs.python.org/file46971/thread-closure-bug-demo.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com