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

A way to determine if a Service closed correctly or errored #66

Closed
seanmonstar opened this issue Apr 13, 2018 · 5 comments
Closed

A way to determine if a Service closed correctly or errored #66

seanmonstar opened this issue Apr 13, 2018 · 5 comments

Comments

@seanmonstar
Copy link
Collaborator

We can check the status of a Service with poll_ready, however, that has only 3 states:

  • ready to call
  • not ready, may be in the future
  • something bad happened

A Service may have closed cleanly, in which case none of those 3 states are correct. The current best choice is to return an error, and hope the error message is sufficient to say it was a clean close. This problem is similar to how before Rust 1.0, a Read closing would return an Err of Eof. Before 1.0, it was changed such that an end-of-file isn't an error, but instead reported as Ok(0).

Some examples of closing cleanly:

  • A connection closed after being idle for too long.
  • A server is shutting down, and wants to trigger graceful shutdown. Notified services would want to report they're closed without an error.

We could similarly make closing a clean Ok status, instead of an error:

// Proposed change
enum Status {
    Open,
    Closed,
}

fn poll_ready(&mut self) -> Poll<Status, Self::Error> {

}

As a side note, it'd be really useful at times to be able to check is_ready and is_closed, without being in a task context. For instance, a wrapped Service that returns it to some pool on Drop could want to check if the underlying Service is closed. Other cases are to allow for call to check if an underlying service is ready even if poll_ready wasn't called previously.

@seanmonstar
Copy link
Collaborator Author

As a comparison, futures mpsc channel has poll_ready as well, and currently encodes the closed state as a Disconnected SendError.

@olix0r
Copy link
Collaborator

olix0r commented Apr 13, 2018

As a part of this change, I think we should explicitly document that poll_ready errors should be considered fatal. If that can't be the case today (i.e. in reconnect), then we may want to figure out if Status needs to handle the case of benign failures.

@seanmonstar
Copy link
Collaborator Author

Reconnect is an interesting case:

  • If the service is established, and svc.poll_ready() is an error, Reconnect swallows it, tosses the svc, and calls new_service().
  • If the Future from new_service itself errors, then Reconnect determines it cannot ever get back up, and returns the connect (NewService::InitError) error.

I don't think the proposed change hurts Reconnect. If anything, it gives it a little more context. It could be adjusted to always reconnect when Status::Closed, but perhaps keep a count of Errs and return after a limit, or something else entirely.

@carllerche
Copy link
Member

Before moving forward with this, I would like to discover a case that is made possible with this change.

A service closing normally can be represented with an Error type today, however there is no standard way to do this. Middleware implementations can always define a trait and require that the inner service's error type implement that trait. In order to justify a change like this, it should provide benefit to the majority of cases.

The original example was a connection pool, where the connection pool needs to know if an inner service failed unexpectedly or closed gracefully. In this example, it is not obvious to me that the connection pool needs to know this. The pool should shield its caller from the details of each individual connection and individual connections may fail without any implication to other connections.

So, I would say this should be a "won't fix" until there is a solid use case driving it.

@carllerche
Copy link
Member

I'm going to close this as it seems that we are leaning towards "won't fix"

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

3 participants