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

Finalize exception names: BrokenResourceError? BusyResourceError? #620

Closed
njsmith opened this issue Aug 22, 2018 · 2 comments
Closed

Finalize exception names: BrokenResourceError? BusyResourceError? #620

njsmith opened this issue Aug 22, 2018 · 2 comments

Comments

@njsmith
Copy link
Member

njsmith commented Aug 22, 2018

This isn't super urgent, but I want to make sure we revisit it before v1.0, so creating an issue that I can tag "potential API breaker".

Should we rename ResourceBusyErrorBusyResourceError, for consistency with ClosedResourceError (and so the most meaningful part of the name come first)? Probably.

Should we consolidate BrokenStreamError, BrokenChannelError, etc., into a generic BrokenResourceError, like we did with ClosedResourceError and ResourceBusyError? Not sure. Maybe. The other ones it was mandatory because these exceptions passed between levels (e.g. socket -> stream). For the Broken family this isn't as likely, so we have the curse of options.

@Zac-HD
Copy link
Member

Zac-HD commented Sep 11, 2018

Should we rename ResourceBusyErrorBusyResourceError, for consistency with ClosedResourceError (and so the most meaningful part of the name come first)? Probably.

Yep, I'd rename it.

Should we consolidate BrokenStreamError, BrokenChannelError, etc., into a generic BrokenResourceError, like we did with ClosedResourceError and ResourceBusyError? Not sure.

I generally design exceptions by asking "what will users want to catch", so why not both? That is, define class BrokenStreamError(BrokenResourceError): so you can either catch any broken resource, or handle specific resource problems if you care about the details.

The argument for BrokenResourceError-only is that we can always compatibly add the subclasses later, so unless we have actual uses for the specific errors in mind I would leave them out for now.

@njsmith
Copy link
Member Author

njsmith commented Sep 28, 2018

why not both?

You meant this rhetorically, but there actually is an answer :-).

Example: suppose we have a "channel" object (#586) that's implemented on top of a Stream, which is implemented on top of a socket. So await channel.send(obj) pickles the object and then calls await stream.send_all(bytes), which in turn calls await sock.send(bytes) (maybe repeatedly).

Now, while that's happening in one task, another task calls await channel.aclose(), which calls await stream.aclose(), which calls sock.close().

sock.send's contract is that when the socket is closed out from under it, it raises ClosedResourceError. stream.send_all's contract is that when the stream is closed out from under it, it raises ClosedResourceError. channel.send's contract is that when the channel is closed out from under it, it raises ClosedResourceError. So... because everyone's contract just happens to use the same exception type, everything automagically works out. The socket implementation has to do some clever inter-task coordination, but for the other classes the naive implementation automatically handles this tricky edge cause correctly. But if we had to translate ClosedSocketError into ClosedStreamError into ClosedChannelError, it would be pretty annoying, and I doubt it would really be that much more useful to users either. (If the exception comes out of channel.send, then obviously it's the channel that was closed! You know what method you called.)

Also, this example with channels on top of streams has kind of convinced me that we should have a single BrokenResourceError (#586 (comment)). (Sockets are already committed to a much more complex API for reporting what SocketStream collapses down to a BrokenStreamError, so the inter-layer argument doesn't really apply to Stream-on-top-of-socket. But it does apply to channel-on-top-of-Stream.)

njsmith added a commit to njsmith/trio that referenced this issue Sep 30, 2018
- Rename BrokenStreamError to BrokenResourceError
- Rename ResourceBusyError to BusyResourceError

Fixes python-triogh-620 (and see there for the rationale).
njsmith added a commit to njsmith/trio that referenced this issue Sep 30, 2018
- Rename BrokenStreamError to BrokenResourceError
- Rename ResourceBusyError to BusyResourceError

Fixes python-triogh-620 (and see there for the rationale).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants