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

Deprecate support for multiple tasks in the private old_nursery used by start() #1600

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

oremanj
Copy link
Member

@oremanj oremanj commented Jun 10, 2020

Fixes #1599; see there (or read the newsfragment) for rationale.

@oremanj oremanj requested a review from njsmith June 10, 2020 01:48
@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #1600 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1600   +/-   ##
=======================================
  Coverage   99.69%   99.69%           
=======================================
  Files         110      110           
  Lines       13849    13858    +9     
  Branches     1056     1057    +1     
=======================================
+ Hits        13807    13816    +9     
  Misses         27       27           
  Partials       15       15           
Impacted Files Coverage Δ
trio/_core/_run.py 99.76% <100.00%> (+<0.01%) ⬆️
trio/_core/tests/test_run.py 100.00% <100.00%> (ø)

@njsmith
Copy link
Member

njsmith commented Jun 15, 2020

Like I mentioned in chat: this might be a good idea, but I think it's hard to tell right now. The unknowns for me are:

  • how much trouble does this actually cause for implementing Idea: service nurseries #1521? That's still in the planning phase, and it's hard to tell this kind of thing without an actual implementation

  • someone finding the nursery through introspection and spawning extra tasks into it is obviously a pretty obscure thing that we don't care that much about supporting. But still, if it happens, we have to do something. And in the current codebase, making it "Just Work" is pretty trivial. OTOH, if we want to raise an error or something, then... what do we do, exactly? We can't just abandon the tasks that aren't supposed to be there...

Hmm. I guess a less-intrusive way to handle this would be to make start_soon on the secret internal start nursery raise an error, similar to if the nursery was closed. Then the issue couldn't arise in the first place.

That feels a lot more "obvious good idea" to me.

@njsmith
Copy link
Member

njsmith commented Jun 20, 2020

So yeah having slept on this a bit I think I'd be fine with a reworked version of this PR that issued the deprecation at the point of spawning a child into that nursery (from spawn_impl), instead of later when it's discovered that there are multiple children running.

@oremanj oremanj marked this pull request as draft June 29, 2020 06:47
@oremanj
Copy link
Member Author

oremanj commented Jun 29, 2020

It's quite a lot of trouble for #1521 to support the case where new service tasks were spawned into the old_nursery. But it would work fine to forbid that case while continuing to allow non-service tasks to come along for the ride. I just figured it was better to be consistent.

Marking this as draft until we come back to #1521.

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.

start() shouldn't support multiple tasks in the old_nursery
2 participants