<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 3, 2018 at 6:35 PM, Nathaniel Smith <span dir="ltr"><<a href="mailto:njs@pobox.com" target="_blank">njs@pobox.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, Jan 3, 2018 at 3:44 PM, Victor Stinner <<a href="mailto:victor.stinner@gmail.com">victor.stinner@gmail.com</a>> wrote:<br>
> Ok, I finally got access to a computer and I was able to test the PEP<br>
> 567 implementation: see my code snippet below.<br>
><br>
> The behaviour is more tricky than what I expected. While running<br>
> context.run(), the context object is out of sync of the "current<br>
> context". It's only synchronized again at run() exit. So<br>
> ContextVar.set() doesn't immediately modifies the "current context"<br>
> object (set by Context.run()).<br>
<br>
</span>To me this sounds like a oversight (= bug), not intended behavior. At<br>
the conceptual level, I think what we want is:<br>
<br>
- Context is a mutable object representing a mapping<br>
- BUT it doesn't allow mutation through the MutableMapping interface;<br>
instead, the only way to mutate it is by calling Context.run and then<br>
ContextVar.set(). Funneling all 'set' operations through a single<br>
place makes it easier to do clever caching tricks, and it lets us<br>
avoid dealing with operations that we don't want here (like 'del')<br>
just because they happen to be in the MutableMapping interface.<br>
- OTOH we do implement the (read-only) Mapping interface because<br>
there's no harm in it and it's probably useful for debuggers.<br></blockquote><div><br></div><div>I think that in essence what Victor saw is a cache consistency issue. If you look at the implementation section in the PEP, the ContextVar.set() operation mutates _ContextData, which is a private (truly) immutable data structure that stands in for the HAMT, and the threadstate contains one of these (not a Context). When you call copy_context() you get a fresh Context that wraps the current _ContextData. Because the latter is effectively immutable this is a true clone. ctx.run() manipulates the threadstate to make the current _ContextData the one from ctx, then calls the function. If the function calls var.set(), this will create a new _ContextData that is stored in the threadstate, but it doesn't update the ctx. This is where the current state and ctx go out of sync. Once the function returns or raises, run() takes the _ContextData from the threadstate and stuffs it into ctx, resolving the inconsistency. (It then also restores the previous _ContextData that it had saved before any of this started.)</div><div><br></div><div>So all in all Context is mutable but the only time it is mutated is when run() returns.<br></div><div><br></div><div>I think Yury's POV is that you rarely if ever want to introspect a Context object that's not freshly obtained from copy_context(). I'm not sure if that's really true; it means that introspecting the context stored in an asyncio.Task may give incorrect results if it's the currently running task.</div><div><br></div><div>Should we declare it a bug? The fix would be complex given the current implementation (either the PEP's pseudo-code or Yury's actual HAMT-based implementation). I think it would involve keeping track of the current Context in the threadstate rather than just the _ContextData, and updating the Context object on each var.set() call. And this is something that Yury wants to avoid, so that he can do more caching for var.get() (IIUC).</div><div><br></div><div>We could also add extra words to the PEP's spec for run() explaining this temporary inconsistency.</div><div><br></div><div>I think changing the introspection method from Mapping to something custom won't fix the basic issue (which is that there's a difference between the Context and the _ContextData, and ContextVar actually only manipulates the latter, always accessing it via the threadstate).</div><div><br></div><div>However there's another problem with the Mapping interface, which is: what should it do with variables that are not set and have no default value? Should they be considered to have a value equal to _NO_DEFAULT or Token.MISSING? Or should they be left out of the keys altogether? The PEP hand-waves on this issue (we didn't think of missing values when we made the design). Should it be possible to introspect a Context that's not the current context?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(Note that I didn't say anything about HAMTs here, because that's<br>
orthogonal implementation detail. It would make perfect sense to have<br>
Context be an opaque wrapper around a regular dict; it would just give<br>
different performance trade-offs.)<br></blockquote></div><br clear="all"></div><div class="gmail_extra">Agreed, that's how the PEP pseudo-code does it.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">-- <br><div class="gmail_signature" data-smartmail="gmail_signature">--Guido van Rossum (<a href="http://python.org/~guido" target="_blank">python.org/~guido</a>)</div>
</div></div>