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

Handle PeerClient timeouts correctly. #51

Closed
hdevalence opened this issue Oct 7, 2019 · 5 comments
Closed

Handle PeerClient timeouts correctly. #51

hdevalence opened this issue Oct 7, 2019 · 5 comments
Assignees

Comments

@hdevalence
Copy link
Contributor

The current peer handling code from #17 does not handle client request timeouts correctly (or at all). This must be fixed. Because we need to statefully translate the legacy protocol into our internal protocol, I think we cannot use the tower_timeout middleware, because we need to reset the PeerServer state when the request is dropped.

There are three ways we could proceed:

  1. "Drop": after a fixed timeout period, if a response has not arrived, fail the peer server and client together, and possibly spin up a new connection to the same remote in a later round of service discovery. This avoids having to change the server's state machine.

  2. "Server Timeout": add a timeout event to the server's state machine that fails a client request after a particular time interval (presumably a configured value set by the PeerConnector).

  3. "Something with Tower": come up with a way to make the server event loop work gracefully with tower_timeout middleware (e.g., detect that the request has been dropped? unsure how this would trigger an event... or if this option is possible).

@hdevalence
Copy link
Contributor Author

This is potentially related to #49 in that we need to have some kind of timeout for ping messages, so if we thread the ping messages through the request mechanism, we need to use request timeouts to tear down the server task.

@hdevalence
Copy link
Contributor Author

Previously I was inclined towards 1) but I think 2) may be better....?

@hdevalence
Copy link
Contributor Author

If we try 2) then this depends on #53 to have a config field.

@hdevalence hdevalence self-assigned this Oct 8, 2019
@hdevalence
Copy link
Contributor Author

Paused this to work on #62, resuming.

@hdevalence
Copy link
Contributor Author

This was done by #65

skyl added a commit to skyl/zebra that referenced this issue Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant