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

don't use f_locals for foreign async generators #3112

Merged

Conversation

graingert
Copy link
Member

@graingert graingert commented Oct 17, 2024

Fixes #2454

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.63%. Comparing base (9d6e134) to head (8fdaaf5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3112   +/-   ##
=======================================
  Coverage   99.62%   99.63%           
=======================================
  Files         122      122           
  Lines       18340    18380   +40     
  Branches     1222     1223    +1     
=======================================
+ Hits        18272    18312   +40     
  Misses         47       47           
  Partials       21       21           
Files with missing lines Coverage Δ
src/trio/_core/_asyncgens.py 100.00% <100.00%> (ø)
src/trio/_core/_tests/test_guest_mode.py 100.00% <100.00%> (ø)

@graingert graingert marked this pull request as ready for review October 17, 2024 18:39
Copy link
Member

@oremanj oremanj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a nice improvement!

as it's critical that self.foriegn.remove runs
Copy link
Member

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and makes sense to me

@graingert graingert marked this pull request as draft October 17, 2024 20:42
@graingert graingert marked this pull request as ready for review October 17, 2024 20:44
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically just nitpicks/comments about the test code. Nothing that should block this from being merged.

src/trio/_core/_tests/test_guest_mode.py Show resolved Hide resolved
src/trio/_core/_tests/test_guest_mode.py Show resolved Hide resolved
@graingert graingert marked this pull request as draft October 17, 2024 21:41
@graingert graingert force-pushed the dont-use-f_locals-for-agen-finalization branch from 8c4d328 to fc1f5bb Compare October 18, 2024 08:31
try:
# If the next thing is a yield, this will raise RuntimeError
# which we allow to propagate
closer.send(None)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually what do we want to happen here if there's a KI? We want long running finalizers to be interruptible, but then the KeyboardInterrupt exception just gets lost so you need to hit Ctrl+C again to terminate trio

Copy link
Contributor

@A5rocks A5rocks Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the KI get lost?

If it's in this closer after we call .send(None), doesn't it get raised here and will propagate up? I assume propagation here is fine because we already have a RuntimeError being raised here.

(I'm basing my assumptions off how generators behave, I don't know anything about async generator cleanup...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The finalizer gets called by tp_finalize which is called by the GC which catches the exception

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... could we catch the KeyboardInterrupt and basically tell trio that it got another Ctrl+C? It seems to be raised normally:

>>> trio.run(main)
Exception ignored in: <async_generator object awaits_after_yield at 0x000001EF4C0F1F00>
Traceback (most recent call last):
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_asyncgens.py", line 120, in finalizer
    closer.send(None)
  File "<stdin>", line 6, in awaits_after_yield
KeyboardInterrupt:

(I also got this great error message in the REPL while playing around with this...)

Funny error message I ran into in the REPL

Exception ignored in: <function Nursery.__del__ at 0x000001EF4BCC54E0>
Traceback (most recent call last):
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 1372, in __del__
    assert not self._children
           ^^^^^^^^^^^^^^^^^^
AssertionError:
Exception ignored in: <coroutine object Runner.init at 0x000001EF4C0239A0>
Traceback (most recent call last):
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 2030, in init
    async with open_nursery() as run_sync_soon_nursery:
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 1030, in __aexit__
    new_exc = await self._nursery._nested_child_finished(exc)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 1192, in _nested_child_finished
    self._add_exc(nested_child_exc)
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 1170, in _add_exc
    self.cancel_scope.cancel()
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_ki.py", line 183, in wrapper
    return fn(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 870, in cancel
    self._cancel_status.recalculate()
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 459, in recalculate
    task._attempt_delivery_of_any_pending_cancel()
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 1545, in _attempt_delivery_of_any_pending_cancel
    self._attempt_abort(raise_cancel)
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 1527, in _attempt_abort
    success = self._abort_func(raise_cancel)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_io_windows.py", line 740, in abort_fn
    self._refresh_afd(base_handle)
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_io_windows.py", line 653, in _refresh_afd
    _check(
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_io_windows.py", line 301, in _check
    raise_winerror()
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_windows_cffi.py", line 520, in raise_winerror
    raise OSError(0, msg, filename, winerror, filename2)
OSError: [WinError 6] The handle is invalid
Exception ignored in: <function Nursery.__del__ at 0x000001EF4BCC54E0>
Traceback (most recent call last):
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 1372, in __del__
    assert not self._children
           ^^^^^^^^^^^^^^^^^^
AssertionError:
Exception ignored in: <function Nursery.__del__ at 0x000001EF4BCC54E0>
Traceback (most recent call last):
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 1372, in __del__
    assert not self._children
           ^^^^^^^^^^^^^^^^^^
AssertionError:
Exception ignored in: <async_generator object awaits_after_yield at 0x000001EF4C0F0040>
Traceback (most recent call last):
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_asyncgens.py", line 88, in finalizer
    runner.entry_queue.run_sync_soon(
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_entry_queue.py", line 145, in run_sync_soon
    self.wakeup.wakeup_thread_and_signal_safe()
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_wakeup_socketpair.py", line 39, in wakeup_thread_and_signal_safe
    self.write_sock.send(b"\x00")
OSError: [WinError 10038] An operation was attempted on something that is not a socket
Exception ignored in: <coroutine object NurseryManager.__aexit__ at 0x000001EF4C01B120>
Traceback (most recent call last):
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 1030, in __aexit__
    new_exc = await self._nursery._nested_child_finished(exc)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\A5rocks\Documents\trio\src\trio\_core\_run.py", line 1220, in _nested_child_finished
    assert popped is self
           ^^^^^^^^^^^^^^
AssertionError:

Not the most clear :^)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be raised normally

But the message says the exception is ignored?

Copy link
Contributor

@A5rocks A5rocks Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could call signal.raise_signal(signal.SIGINT) in the except KeyboardInterrupt: and call it a day? Not a great solution IMO. (though actually that ... would probably raise in that except statement. ouch? nevermind)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. The except KeyboardInterrupt would have to be outside the _finalize_without_ki_protection

Copy link
Contributor

@A5rocks A5rocks Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this awful hack?:

except KeyboardInterrupt:
  @enable_ki_protection
  def killer():
    signal.raise_signal(signal.SIGINT)
  try:
    killer()
  except KeyboardInterrupt:
    # turns out the trio KI handler isn't installed. Nothing good to do
    raise KeyboardInterrupt from None

edit: sorry, your comments weren't showing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what I'm going to do here is put the KI protection on all the critical and guaranteed fast trio code and unprotect any of the host loop stuff. Then we can survey someone who uses guest mode and KI and async generators without aclose and find out what they want

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this awful hack?:

except KeyboardInterrupt:
  @enable_ki_protection
  def killer():
    signal.raise_signal(signal.SIGINT)
  try:
    killer()
  except KeyboardInterrupt:
    # turns out the trio KI handler isn't installed. Nothing good to do
    raise KeyboardInterrupt from None

Another KI could arrive while constructing or decorating or load_fast-ing the killer function

@graingert graingert marked this pull request as ready for review October 28, 2024 07:41
@A5rocks
Copy link
Contributor

A5rocks commented Nov 1, 2024

Oh the pypy cache failures are back :(

I tried deleting the relevant caches from the GH actions cache page last time this happened but I guess they came back. That still works to fix it temporarily though. (Or they get fixed after a few hours for some reason)

@graingert graingert merged commit 34e7a2e into python-trio:main Nov 21, 2024
37 of 38 checks passed
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

Successfully merging this pull request may close these issues.

use of locals() / f_locals() foils pypy
4 participants