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

Tidy up error variants #15

Closed
cloudhead opened this issue Jan 25, 2023 · 1 comment · Fixed by #19
Closed

Tidy up error variants #15

cloudhead opened this issue Jan 25, 2023 · 1 comment · Fixed by #19
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@cloudhead
Copy link
Contributor

cloudhead commented Jan 25, 2023

There are currently too many variants in reactor::Error, which makes the crate very hard to use, as it's unclear how each of these errors should be handled. Ideally most of the variants have clear actionables, eg. unregistering the socket. Here's what I'd start with.


These errors come from improper use of the API by the user, and shouldn't be reactor errors.
If we're trying to unregister an unknown peer, not returning an error makes unregistering idempotent,
which makes the API easier to use. If we're trying to write to an unknown peer, we could return a
WriteFailure error instead. Since they aren't fatal and can easily happen due to race conditions, I'd simply log an error
and remove the variants:

reactor::Error::ListenerUnknown(id)
reactor::Error::TransportUnknown(id)
reactor::Error::WriteLogicError(id, _)

These are pretty critical and it's not clear how they can happen, but if our listener socket goes
down, we probably want to restart the node. I would probably merge the two variants into Error::ListenerFailed,
since there isn't really a difference in how they should be handled.

reactor::Error::ListenerPollError(id, err)
reactor::Error::ListenerDisconnect(id, _, err)

This error should be whatever the internal Poll implementation returns. Ideally it can be downcasted to the
concrete error type. Poll errors should be fatal, so we'd probably abort here.

reactor::Error::Poll(err)

I'm guessing this one is a clean disconnect, in which case we can handle it and unregister the socket:

reactor::Error::TransportDisconnect(id, _, err)

These two seem like they can be bundled under Error::Transport, since they'd likely both be handled by
disconnecting:

reactor::Error::TransportPollError(id, err)
reactor::Error::WriteFailure(id, err)
@dr-orlovsky dr-orlovsky self-assigned this Jan 25, 2023
@dr-orlovsky dr-orlovsky added the enhancement New feature or request label Jan 25, 2023
@dr-orlovsky dr-orlovsky added this to the v0.2.0 milestone Jan 25, 2023
@dr-orlovsky
Copy link
Member

dr-orlovsky commented Jan 27, 2023

The difference between these tow

reactor::Error::ListenerPollError(id, err)
reactor::Error::ListenerDisconnect(id, _, err)

and these two

reactor::Error::TransportDisconnect(id, _, err)
reactor::Error::TransportPollError(id, err)

is that *Disconnect variants unregister resource from the reactor automatically, while *PollError do not. My assumption was that disconnect is permanent while PollError or write errors may be temporary

Not sure this is a best strategy, but it was a reason for having distinct error types

@dr-orlovsky dr-orlovsky linked a pull request May 11, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants