When a socket/fd is closed, wake up outstanding waiters

[njsmith]

Still todo:

• full test coverage
• updating the stream layer to match
• is InterruptedByCloseError the best name? Should it inherit OSError?
• Which layers should use which exception?

Fixes gh-36, gh-459

[njsmith]

CC @sscherke @smurfix, since I know this bit both of you before.

[njsmith]

Sorry, I mean @sscherfke

[codecov]

Codecov Report

Merging #460 into master will decrease coverage by 0.01%.
The diff coverage is 98.92%.

@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
- Coverage   99.28%   99.27%   -0.02%     
==========================================
  Files          89       89              
  Lines       10499    10617     +118     
  Branches      728      747      +19     
==========================================
+ Hits        10424    10540     +116     
- Misses         58       59       +1     
- Partials       17       18       +1

Impacted Files	Coverage Δ
trio/_toplevel_core_reexports.py	100% <ø> (ø)	⬆️
trio/_highlevel_generic.py	100% <ø> (ø)	⬆️
trio/_abc.py	100% <ø> (ø)	⬆️
trio/hazmat.py	100% <ø> (ø)	⬆️
trio/_core/__init__.py	100% <100%> (ø)	⬆️
trio/_core/_io_windows.py	77.09% <100%> (+1.07%)	⬆️
trio/_core/_exceptions.py	100% <100%> (ø)	⬆️
trio/_highlevel_socket.py	100% <100%> (ø)	⬆️
trio/tests/test_ssl.py	100% <100%> (ø)	⬆️
trio/_core/_io_epoll.py	100% <100%> (ø)	⬆️
... and 10 more

---

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 79d6664...75e74d7. Read the comment docs.

[sscherfke]

I tried your branch and my examples are working now. :-)

[njsmith]

I think the way to do this is to rename ClosedStreamError and ClosedListenerError to ClosedResourceError (or just ClosedError?), and then use that same exception here too, to avoid cross-layer skew.

So todo list:

• renames + deprecation + release notes for same
• update code to use it instead of InterruptedByCloseError
• add tests
• fix the testing streams to implement the same behavior
• update the generic stream tests to check for this

[smurfix]

Hmm. ClosedResourceError, as a subclass of ClosedError, as a subclass of RuntimeError?

[sscherfke]

Why not ConnectionResetError? It’s built in and people are used to it.

[njsmith]

@smurfix We already use Resource to mean "anything that can be closed", so I'm not sure how you could have a ClosedError that wasn't also a ClosedResourceError :-).

@sscherfke ConnectionResetError is part of Python's OSError hierarchy. They don't document this as well as they could, but it means that ConnectionResetError has very specific semantics: "I did some syscall, and it returned with errno = ECONNRESET." That's not what happened here, and in fact it's a different situation: at the abstract stream level we don't expose errnos and stuff, but just distinguish between "some external event broke this stream" and "you broke this stream by calling close". (Because this is the only distinction that makes sense for a generic stream that might have some arbitrary underlying implementation, and also because mostly users don't care about any details beyond this anyway.) ECONNRESET is in the former category (it means a TCP peer sent us a RST packet), while this PR is about what exception to use for the latter category :-).

[njsmith]

Just pushed a much more complete patch here.

Still todo:

• Make sure cross-platform tests and coverage look good.
• Make a final decision on exception names. I'm leaning towards renaming ResourceBusyError to BusyResourceError, so that our exceptions consistently put the most distinctive information first. I'm wondering whether we should have a BrokenResourceError, that BrokenStreamError could be replaced by (or subclass I guess); right now it really is specific to streams, but we've had to move two different exceptions out of the stream API now (BusyResourceError and ClosedResourceError) in order to avoid skew between levels of abstraction, so maybe we should be taking a hint...

This is in a good state to review though.

[oremanj]

Looks great! A moderate level of scrutiny didn't find any gaps in logic; just one documentation bit for your consideration.

[oremanj]

Do you think it's worth also mentioning the "exited a context manager" possibility for why the thing was closed?

[njsmith]

Yeah, that's a good idea!

[njsmith]

It looks like the new uncovered lines are in the logic for handling monitor_kevent, which is an untested stub anyway, so not an issue here.

[njsmith]

@Fuyukai raised an interesting question in chat: you might expect __del__ methods to call notify_*_close, and what happens then? The problem is that invoking any kind of trio methods from __del__ is fraught, since __del__ is like a signal handler that might run at an arbitrary moment, or in an arbitrary thread. The only Trio function that's safe to call from a __del__ method is TrioToken.run_sync_soon. So... we could define __del__ methods that do that, e.g. on trio sockets.

This patch doesn't, though. Why not? Well... if you're calling wait_* on a socket, then you'd better be holding a reference to that socket! And in fact the wait_socket_* functions actually do hold a reference to the socket that's passed in. So... __del__ won't run while there are any wait_socket_* calls outstanding. Or put another way, if __del__ is being called, then it means we don't need to worry about calling notify_socket_close.

There might be some super-obscure edge cases where this breaks down? But implementing and testing __del__ would be super tricky, and I can't think of any now, so I think we can let it be and worry about it if it ever comes up (or if someone comes up with a case where it's actually a problem).

[njsmith]

@Fuyukai also pointed out that we can always handle further exception name changes as a separate PR. So I think I've now addressed all comments and this is ready to merge.

[njsmith]

@oremanj thanks!