Disable cancel scope reuse #911
Conversation
To keep our options open for adding a smarter `was_cancelled` property, per discussion in python-trio#886.
Codecov Report
@@ Coverage Diff @@
## master #911 +/- ##
=========================================
- Coverage 99.4% 99.4% -0.01%
=========================================
Files 102 102
Lines 12681 12678 -3
Branches 927 927
=========================================
- Hits 12605 12602 -3
Misses 56 56
Partials 20 20
|
trio/_core/_run.py
Outdated
| " in another task ({!r})".format(self._scope_task.name) | ||
| ) | ||
| "cancel scopes may not be reused or reentered " | ||
| "(first used in task {!r})".format(self._scope_task.name) | ||
| ) | ||
| self._scope_task = task |
There was a problem hiding this comment.
Do we use self._scope_task for anything else now? If it's effectively a bool, it would probably be better to make an an actual bool, so we don't inadvertently hold onto any stack frames longer than we need to.
There was a problem hiding this comment.
We use it in _exc_filter (testing whether self._scope_task._pending_cancel_scope() is self). I also don't think holding onto a Task can actually leak much memory -- if the task is running, it's referenced by the runner so our reference doesn't change the memory usage, and if it's done, it doesn't have any stack frames so the memory usage is small. Is there something I'm missing?
There was a problem hiding this comment.
Maybe CancelScope._exc_filter should use current_task()? ...and maybe it should be refactored into CancelScope._close. And actually, now that all Cancelled exceptions are the same, we can hoist the check up out of _exc_filter entirely; no reason _close should call MultiError.filter at all if it's not going to catch anything. That's another matter though...
I guess you're right that coroutine objects drop their cr_frame reference when the coroutine exits. Huh, I didn't know that. That does reduce the likelihood of this reference causing problems. So now it still makes me nervous to hold onto an unnecessary reference to a complex object, but it's only on general principles, not because of any specific issue I can point to :-).
|
Huh, I would have added a |
|
I figured it would be one fewer slot. I'll add the separate bool. |
|
Azure Ubuntu timeout again, same reason as last night. |
|
Even slower on a rerun. ... are we getting any value out of the lone Linux test environment on Azure? Should we just disable it? |
Unfortunately yes, it's our only environment that has openssl 1.1.1, and so far our experience has been that 1.1.1 is cranky and easy to break. We could move it to Travis, or bump up the timeout as a temporary workaround... It's super weird that the two times this has happened were both at 10:00-11:00 PM west coast time. I wonder if that's accidental. |
I don't think so: I think Azure Ubuntu is getting popular, and this is going to happen every day. |
|
Ah, that explanation is better! 👍 Maybe they have cron jobs running at midnight. |
|
Hmm, actually, it's not quite at midnight. The first time we had this problem was 2019-02-05 (UTC):
So this episode started somewhere between 05:57 and 06:39 UTC, and ended somewhere between 07:07 and 07:35 UTC. The next day, 2019-02-05 UTC, we didn't run any builds between Then then the day after that puts us at today, 2019-02-06 UTC:
So this time the slowdown started between 06:15 and 06:35 UTC, and ended around 07:15 UTC. That is still remarkably consistent with the previous window. I also peeked back one day into the past, and it turns out that we did do a build at [Edit: it just happened again, this time at 2019-02-14 06:36 UTC.] |
To keep our options open for adding a smarter
was_cancelledproperty, per discussion in #886.