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

Reduce extraneous exception frames #640

Merged
merged 5 commits into from Sep 2, 2018
Merged

Conversation

@belm0
Copy link
Member

@belm0 belm0 commented Sep 1, 2018

For #56:

  • _nested_child_finished() returns rather than raises MultiError
  • implement explicit context manager for CancelScope rather than @contextmanager
  • propagate exceptions from cancel scope to nursery __aexit__ manually

TODO:

  • fix test_nursery_exception_chaining_doesnt_make_context_loops() test error
  • remove more frames by propagating exceptions from cancel scope to nursery __aexit__ manually
  • fix test_slow_abort_edge_cases() etc. test errors
@belm0 belm0 requested a review from njsmith Sep 1, 2018
@belm0
Copy link
Member Author

@belm0 belm0 commented Sep 1, 2018

status of our reference exception:

Traceback (most recent call last):
  File "demo.py", line 58, in <module>
    test_multiplexer_with_error()
  File "demo.py", line 55, in test_multiplexer_with_error
    trio.run(runner2)
  File "/.../site-packages/trio/_core/_run.py", line 1314, in run
    return result.unwrap()
  File "/.../site-packages/outcome/_sync.py", line 107, in unwrap
    raise self.error
  File "/.../site-packages/trio/_core/_run.py", line 1004, in init
    self.entry_queue.spawn()
  File "/.../site-packages/trio/_core/_run.py", line 362, in __aexit__
    type(new_exc), new_exc, new_exc.__traceback__
  File "/.../site-packages/trio/_core/_ki.py", line 162, in wrapper
    return fn(*args, **kwargs)
  File "/.../site-packages/trio/_core/_run.py", line 230, in __exit__
    raise filtered_exc
  File "demo.py", line 52, in runner2
    nursery.start_soon(writer2, mx, (7,9))
  File "/.../site-packages/trio/_core/_run.py", line 362, in __aexit__
    type(new_exc), new_exc, new_exc.__traceback__
  File "/.../site-packages/trio/_core/_ki.py", line 162, in wrapper
    return fn(*args, **kwargs)
  File "/.../site-packages/trio/_core/_run.py", line 230, in __exit__
    raise filtered_exc
  File "demo.py", line 15, in reader
    raise e
  File "demo.py", line 9, in reader
    value = await mx[key]
  File "multiplexer.py", line 41, in __getitem__
    value = await trio.hazmat.wait_task_rescheduled(abort_fn)
  File "/.../site-packages/trio/_core/_traps.py", line 165, in wait_task_rescheduled
    return (await _async_yield(WaitTaskRescheduled(abort_func))).unwrap()
  File "/.../site-packages/outcome/_sync.py", line 107, in unwrap
    raise self.error
Exception: Ka-Boom!

Loading

@belm0
Copy link
Member Author

@belm0 belm0 commented Sep 1, 2018

Reference exception after the reworked cancel scope-to-nursery exit exception path (unfortunately at the cost of another failure in test_run.py):

Traceback (most recent call last):
  File "demo.py", line 58, in <module>
    test_multiplexer_with_error()
  File "demo.py", line 55, in test_multiplexer_with_error
    trio.run(runner2)
  File "/.../site-packages/trio/_core/_run.py", line 1316, in run
    return result.unwrap()
  File "/.../site-packages/outcome/_sync.py", line 107, in unwrap
    raise self.error
  File "/.../site-packages/trio/_core/_run.py", line 1006, in init
    self.entry_queue.spawn()
  File "/.../site-packages/trio/_core/_run.py", line 372, in __aexit__
    raise scope_exc
  File "demo.py", line 52, in runner2
    nursery.start_soon(writer2, mx, (7,9))
  File "/.../site-packages/trio/_core/_run.py", line 372, in __aexit__
    raise scope_exc
  File "demo.py", line 15, in reader
    raise e
  File "demo.py", line 9, in reader
    value = await mx[key]
  File "multiplexer.py", line 41, in __getitem__
    value = await trio.hazmat.wait_task_rescheduled(abort_fn)
  File "/.../site-packages/trio/_core/_traps.py", line 165, in wait_task_rescheduled
    return (await _async_yield(WaitTaskRescheduled(abort_func))).unwrap()
  File "/.../site-packages/outcome/_sync.py", line 107, in unwrap
    raise self.error
Exception: Ka-Boom!

Loading

@belm0 belm0 force-pushed the nursery_aexit_raise branch from 1cb40da to a309a76 Sep 2, 2018
@njsmith
Copy link
Member

@njsmith njsmith commented Sep 2, 2018

This changes the public API in a few ways:

  • new public method CancelScope.create, though it's a weird method: CancelScope itself is still private, but if you get access to a CancelScope object, then you can call cancel_scope.create(...) to make a new CancelScope object.
  • open_cancel_scope returns a CancelScope object, so it becomes possible to do cancel_scope = open_cancel_scope() in one place, and with cancel_scope: ... somewhere else, like proposed in #607
  • if you have any cancel scope object, you can now write with cancel_scope: ... to... make something happen. Not sure if this counts as a new public API, because if the cancel scope was already used with a with somewhere else, than this will end up corrupting internal state (cancel_scope._scope_task gets overwritten and eventually things fall apart)

I think we do want to eventually make the changes proposed in #607, and this gets us closer, so that's good. But the API changes are going to take more finagling (e.g. deprecating some stuff, and the proposal there has CancelScope.__enter__ return None, ...), and I don't want this to grow to try to solve both sets of things at once and get stalled. So my suggestion is that in this PR we avoid making any visible API changes and then we can build on it later.

So specifically:

  • CancelScope.create should become CancelScope._create

  • open_cancel_scope should remain a @contextmanager, and do:

    cancel_scope = ...
    with cancel_scope:
        yield cancel_scope

    (I know this adds tb frames, but only when people use cancel scopes directly, not on the nursery path; and they'll go away again when we finish the API changes and deprecate open_cancel_scope. And it keeps users from getting access to unbound CancelScope objects until we're ready.)

  • In CancelScope.__enter__, add an assert self._scope_task is None check, so if anyone does accidentally try to with cancel_scope: ... they'll at least notice. (We can replace it with a proper check when we implement #607 for real; this is just to make sure that if someone makes this mistake they get an error quickly instead of slowly.)

Loading

@belm0
Copy link
Member Author

@belm0 belm0 commented Sep 2, 2018

Thank you. I made CancelScope itself the context manager only for implementation convenience. Instead I could make a separate CancelScopeManager (if indeed that saves frames vs. @contextmanager). Yep, not using _create was an oversight.

Loading

@njsmith
Copy link
Member

@njsmith njsmith commented Sep 2, 2018

Well, we are planning to make CancelScope a context manager shortly, so it's not a big deal to put the code there now – if you make a CancelScopeManager here we'll just end up moving the code back to CancelScope anyway :-). We just want to keep it behind the curtain, so to speak, until we're ready to unveil it.

Loading

than @contextmanager to avoid extraneous exception frames.
@belm0 belm0 force-pushed the nursery_aexit_raise branch from a309a76 to a12998c Sep 2, 2018
@belm0
Copy link
Member Author

@belm0 belm0 commented Sep 2, 2018

It's trivial to move the enter/exit between CancelScopeManager and CancelScope (and we now have a commit to refer to for exactly that when the time comes), so I went with CancelScopeManager over @contextmanager.

I've renamed to _create(), so all that's left is the failing test.

Loading

@njsmith
Copy link
Member

@njsmith njsmith commented Sep 2, 2018

the failing test.

Strange – this says:

=============== 5 failed, 429 passed, 8 skipped in 32.00 seconds ===============

In all cases, the crash appears to be when a task finishes, and we walk through its cancel stack unregistering the task from all the cancel scopes... and one of the cancel scopes is like "what? this task wasn't registered in the first place". So like... somehow the task's list of scopes its inside, the the scope's list of tasks inside it, are getting out of sync.

Loading

trio/_core/_run.py Outdated Show resolved Hide resolved
Loading
belm0 added 2 commits Sep 2, 2018
To further reduce extraneous exception frames, CancelScope gets
create() constructor and close() methods, which nursery __aexit__
uses directly rather than going through a scope context manager.
@belm0 belm0 force-pushed the nursery_aexit_raise branch from a12998c to 2ed38fb Sep 2, 2018
@codecov
Copy link

@codecov codecov bot commented Sep 2, 2018

Codecov Report

Merging #640 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #640      +/-   ##
==========================================
+ Coverage   99.31%   99.31%   +<.01%     
==========================================
  Files          93       93              
  Lines       10968    10985      +17     
  Branches      783      786       +3     
==========================================
+ Hits        10893    10910      +17     
  Misses         56       56              
  Partials       19       19
Impacted Files Coverage Δ
trio/_core/_run.py 99.7% <100%> (ø) ⬆️
trio/_core/tests/test_run.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a27696b...d6fee5d. Read the comment docs.

Loading

@njsmith
Copy link
Member

@njsmith njsmith commented Sep 2, 2018

Reference exception

This is so, so awesome.

Can you add a newsfragment, I guess 56.feature.rst, "tracebacks are good now"? (Ok not literally that, but... basically that.)

Loading

@njsmith njsmith merged commit 7265ca2 into python-trio:master Sep 2, 2018
5 checks passed
Loading
@njsmith njsmith mentioned this pull request Sep 2, 2018
11 tasks
@belm0 belm0 deleted the nursery_aexit_raise branch Sep 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants