Should aclose() always wrap its contents in a CancelScope(shield=True)?

[jwodder]

The docs say:

If you need to use await during finalization, you need to enclose it in a shielded cancel scope, or the operation will be cancelled immediately since it’s in an already cancelled scope

Since aclose() on an AsyncResource gets called whenever an exception is bubbling up (which, as I understand it, includes cancellation errors), it would seem that we would always want to use with CancelScope(shield=True) inside an aclose(). (Indeed, doing this appears to have been the solution to a strange error I was just facing that was causing an @asynccontextmanager containing nested async context managers to not yield during cancellation.) Should this be standard practice? Even better, should AsyncResource.__aexit__ wrap its call to aclose() in a shield?

[smurfix]

Even better, should AsyncResource.__aexit__ wrap its call to aclose() in a shield?

It probably should; the problem is, what if aclose blocks indefinitely, e.g. because it's sending a Close handshake and the peer has become unreachable?

My SOP is to use a shield with a two-second-or-whatever timeout. Large enough to complete such a handshake, but too small to trigger my "hit ^C a few more times, then suspend and kill -9 the thing" reflex.

[njsmith]

Trio's convention is that:

• aclose methods must always succeed, regardless of whether they're called in a cancelled context or not
• if they're called in a cancelled context, they must return promptly (so e.g. skip the Close handshake)
• otherwise they can do a graceful close if that's appropriate

So if you're using this convention, you don't want to shield the call to aclose, because that denies aclose the ability to tell whether it's supposed to do a graceful shutdown or not. It does require that everyone writing aclose methods follows the rules, though.

[graingert]

(Indeed, doing this appears to have been the solution to a strange error I was just facing that was causing an @asynccontextmanager containing nested async context managers to not yield during cancellation.)

do you have an example of what this was? not yielding during cancellations also sounds wrong

[jwodder]

Alas, I tried to produce an MVCE a couple times, but I was unable to get something that reproduced the error.

[parity3]

we would always want to use with CancelScope(shield=True) inside an aclose().

Wouldn't you want the shielded cancel scope to be set up in the outside caller (the one calling await aclose())? Not sure how you'd be able to wrap that behavior nicely.

When the outer await happens, that's a cancel point even if the called method doesn't actually await, if I understand correctly. And that outer await would need to be shielded for aclose() to run at all.