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

Exceptions from QUIC APIs should derive from IOException #56954

Closed
geoffkizer opened this issue Aug 6, 2021 · 6 comments
Closed

Exceptions from QUIC APIs should derive from IOException #56954

geoffkizer opened this issue Aug 6, 2021 · 6 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Net.Quic
Milestone

Comments

@geoffkizer
Copy link
Contributor

For one, exceptions from streams usually derive from IOException and QuicStream should follow this.

But it also seems reasonable and consistent to have exceptions from QuicConnection/QuicListener derive from IOException as well. In particular, some exceptions are used for both QuicStream and QuicConnection.

Related: #56292 (comment)

@geoffkizer geoffkizer added this to the 7.0.0 milestone Aug 6, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 6, 2021
@ghost
Copy link

ghost commented Aug 6, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

For one, exceptions from streams usually derive from IOException and QuicStream should follow this.

But it also seems reasonable and consistent to have exceptions from QuicConnection/QuicListener derive from IOException as well. In particular, some exceptions are used for both QuicStream and QuicConnection.

Related: #56292 (comment)

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Quic

Milestone: 7.0.0

@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Aug 6, 2021
@GSPP
Copy link

GSPP commented Aug 7, 2021

Precedent is mostly that IOException is not being used as a base class. Rather, QuicException should be caught by calling code.

@geoffkizer
Copy link
Contributor Author

The problem is that QuicStream derives from Stream, and in general Stream throws IOException. So at least for QuicStream, it seems like we should derive from IOException. so that code that is operating on a generic Stream can at least distinguish IOExceptions from other exceptions.

That said, I don't know how consistent we are about this in general with Stream. Maybe it's not worth it. @stephentoub thoughts?

@stephentoub
Copy link
Member

I think it's fine to derive from IOException, though we can discuss it with API review when all of this comes through. There's a lot of antiquated exception derivation stemming from decisions 20 years ago, e.g. the closest existing counterpart to QuicException is probably SocketException, but we're obviously not going to make QuicException derive from Win32Exception or SystemException. There are plenty of exceptions that derive from IOException, and it is what's propagated from types like NetworkStream.

@karelz karelz added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Nov 16, 2021
@nibanks
Copy link

nibanks commented Apr 20, 2022

What about any exceptions/errors related to invalid parameters? TLS/security/crypto related errors? I'm not sure all QUIC errors should be considered IOExceptions necessarily. But I guess it's more of a question: Does the base class of an exception reflect the type of exception or source of exception?

@ManickaP
Copy link
Member

Closing as covered by #70669

@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Net.Quic
Projects
None yet
Development

No branches or pull requests

6 participants