trio icon indicating copy to clipboard operation
trio copied to clipboard

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

Open oremanj opened this issue 5 years ago • 4 comments

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

oremanj avatar Jun 10 '20 01:06 oremanj

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%) :arrow_up:
trio/_core/tests/test_run.py 100.00% <100.00%> (ø)

codecov[bot] avatar Jun 10 '20 01:06 codecov[bot]

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 #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 avatar Jun 15 '20 11:06 njsmith

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.

njsmith avatar Jun 20 '20 08:06 njsmith

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.

oremanj avatar Jun 29 '20 06:06 oremanj