Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dict don't like that LOCALS_KEY_KI_PROTECTION_ENABLED is not a string... #89

Closed
Carreau opened this issue Mar 11, 2017 · 5 comments
Closed

Comments

@Carreau
Copy link
Contributor

Carreau commented Mar 11, 2017

Ok, I can take care of that in IPython and have it working... still:

In [2]: await trio.sleep(2)
intermediate : ({'.... dont': 'care ....', '__name__': '__main__', <class 'trio._core._ki.LOCALS_KEY_KI_PROTECTION_ENABLED'>: False}, None)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
/Users/bussonniermatthias/dev/ipython/IPython/core/interactiveshell.py in _async_exec(self, code_obj, user_ns, loop_runner)
   3007             raise ValueError('Async Code may not modify copy of User Namespace and need to return a new one')
   3008         user_ns.clear()
-> 3009         user_ns.update(**loop_ns['user_ns'])
   3010         return loop_ns['last_expr']
   3011

TypeError: keyword arguments must be strings

In [3]:

Also you seem to use locals()[LOCALS_KEY_KI_PROTECTION_ENABLED] = True in a couple of place.

I'm unsure how you can make it work knowing how Python does name lookup in function:

$ python
Python 3.6.0 | packaged by conda-forge | (default, Feb 10 2017, 07:08:35)
[GCC 4.2.1 Compatible Apple LLVM 7.3.0 (clang-703.0.31)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> def foo():
...     locals()['wow'] = 1
...     print(wow)
...
>>> foo()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in foo
NameError: name 'wow' is not defined
>>>

I'm going to guess this is likely done on purpose. Just just trying to understand.

@njsmith
Copy link
Member

njsmith commented Mar 11, 2017

oh UGH

yeah, this is one awful workaround for python limitations colliding with another awful workaround for python limitations.

the short answer is that you can work around it by having your trio runner wrap the synthetic function inside a do-nothing pass-through function:

async def call(async_fn, *args, **kwargs):
    return await async_fn(*args, **kwargs)

trio.run(call, actual_async_fn)

(I guess you could sort out your problem with kwargs here too actually, kill 2 birds with 1 stone.)

The long answer is: handling control-C correctly is hard, because if it arrives while we're running user code (e.g. that infinite loop that you accidentally wrote, oops) then we want KeyboardInterrupt to be raised immediately, but if we're in the guts of trio then we definitely don't want KeyboardInterrupt to be raised because that could corrupt our internal state and break everything. So trio installs a signal handler for SIGINT, and when the user hits control-C the handler has to figure out whether we're in user code or trio internals. The only possible way to do this is by walking the stack looking for a marker saying "entering user code" or "entering trio code", and it turns out that the only thing in a stack trace that we have control over and can use as a marker is: the frame's locals. (This is ugly and still subject to a small unfixable race condition in some cases; IMO python frames should hold a reference to the function object so we can check it for attributes, see #79. But for now this is what we're stuck with.)

So currently our marker is a non-string object, to make sure that we can't possibly accidentally collide with any real local variable, and this gets injected into the top frame of every (non-system) task. (And this is why adding a trivial wrapper function will let you dodge the issue – we'll inject it into the wrapper function's stack frame, rather than the user code's frame.)

You definitely don't want to copy this into the IPython user namespace... I can't actually think of anything that would break, but given the weird stack walking stuff we have to do, letting this variable leak into other parts of the stack is not going to promote stability and predictability.

@njsmith
Copy link
Member

njsmith commented Mar 11, 2017

Oh, and the reason why assigning to locals() is OK here is that variables assigned via locals() are visible to other calls to locals() or the frame introspection APIs – it's only the STORE_FAST and LOAD_FAST opcodes that go through the locals cache instead of checking the dict. And here we only care about it being visible to via frame introspection.

@njsmith
Copy link
Member

njsmith commented Mar 11, 2017

(I'm planning to do a blog post soon about trio's control-C handling, it's actually a really interesting topic)

@Carreau
Copy link
Contributor Author

Carreau commented Mar 11, 2017

All make sens, thanks.

If you need a proofreader or feedback before publishing I'll be happy to have a look.
The async-def no-op wrapper works, I'm still wondering if I should add extra protections when updating user_ns. Right now I was filtering the keys by types. I'll remove it and let things crash if it does not work.

@njsmith
Copy link
Member

njsmith commented Mar 15, 2017

I guess this can be closed, or...?

@njsmith njsmith closed this as completed Aug 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants