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

mgnp: better error handling #23

Open
hawkw opened this issue Nov 10, 2023 · 2 comments
Open

mgnp: better error handling #23

hawkw opened this issue Nov 10, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@hawkw
Copy link
Contributor

hawkw commented Nov 10, 2023

right now (as of 87dc51d) we handle frame decode/encode errors by "just skipping the frame", and wire errors by "just stop running the interface". here's what we should do instead, IMO:

  • on outbound frame serialize errors, do: send an error back to the local conn that sent that frame
  • on inbound frame decode errors, do:
    • if we were able to decode the header and get a link ID, then: close the connection for that link ID (send a RESET to the remote/an error to the local channel, or a NAK if it was a connection being established?) and remove it from the conn table
    • else if we weren't able to decode the header and get a link ID, then: either destroy the whole interface and reset all conns, or...log an error and ignore it? idk @jamesmunns what do you think?
  • on outbound frame wire send errors, do:
    • if it was a DATA or CONNECT frame, then: send an error back to the local conn?
    • if it was an ACK, NAKor RESET then: we're probably fucked, blow up the whole link?
    • if we were trying to send a RESET due to a previous error, then: hahaha we're Super Fucked, destroy everything as violently as possible
  • on inbound frame wire recv errors, do: idk, either ignore it or destroy the whole world probably? @jamesmunns what do you think?

imo if the Wire sees an error that it's capable of retrying or believes to be ignorable, it should just ... do that? and the only errors that should bubble up from the Wire are fatal-ish and should blow up the whole interface.

currently, wire errors just make us stop the interface state machine. they should actually send errors on all existing local connections.

@hawkw hawkw added the enhancement New feature or request label Nov 10, 2023
@hawkw
Copy link
Contributor Author

hawkw commented Nov 11, 2023

some notes about the design given here:

  • because decode errors result in RESET/NAKing a whole connection, rather than NAKing frames (which MGNP has no concept of), we don't need frame sequence numbers so that individual frames can be NAKed
  • since we don't retransmit bad frames, no buffering/queueing is required
  • alternatively, we may want to consider a design where, if we get a bad frame with a good ID, we just tell the local side of the connection "hey the remote gave you something bad", and let the service decide if it wants to blow up that connection?

@hawkw
Copy link
Contributor Author

hawkw commented Nov 12, 2023

i think the best way to propagate connection RESETs is to stick a first-class notion of "channel closed with error" into tricky-pipe.

also, it would be nice if an attempt to send a message through a DeserSender that results in a deserialize error also sent the deserialize error to the receiver, as well as returning it to the sender...

hawkw added a commit that referenced this issue Nov 22, 2023
This branch implements connection-level resets, as described in #23. 

In order to implement resets, I've added support for closing a
`tricky-pipe` channel with a typed error. Both the sender and receiver
can close the channel with an error, and when this occurs, both sides
will return that error when receiving/sending. This allows us to easily
propagate connection-level resets from the peer to a local client/server
when a reset message is received on the wire.

I have *not* implemented global resets on wire-level errors yet. I plan
to do that in a subsequent PR.
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

No branches or pull requests

1 participant