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

Include connection info in Client::send_request errors #2749

Merged
merged 2 commits into from
Apr 14, 2023

Conversation

nox
Copy link
Contributor

@nox nox commented Feb 4, 2022

No description provided.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I don't know yet what the best outcome here is. I think including some info in the error is probably a good idea. I think there's some prior art in other libraries, and some potential new features in libstd that we may wish to consider, before refining what exactly we expose.

@@ -206,9 +211,20 @@ impl Error {
self.inner.cause
}

/// Returns the info of the client connection on which this error occurred.
#[cfg(all(feature = "client", any(feature = "http1", feature = "http2")))]
pub fn client_connect_info(&self) -> Option<&Connected> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize they're not implemented yet, but this made me think of the errors RFCs about "generic context" access, and would seem like a good fit eventually. rust-lang/rfcs#2895 and rust-lang/rfcs#3192

@@ -242,7 +242,7 @@ where
if req.version() == Version::HTTP_2 {
warn!("Connection is HTTP/1, but request requires HTTP/2");
return Err(ClientError::Normal(
crate::Error::new_user_unsupported_version(),
crate::Error::new_user_unsupported_version().with_client_connect_info(pooled.conn_info.clone()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also kind of reminds me of how in Finagle, the exceptions/failures there have "sources" and/or "remote info".

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanmonstar, not familiar with finagle... Can you point to the library doc or impl. Also would be informative to know an example use you considered best practice or at least idiomatic.
Context: I'm refactoring the traceable errors in my tracing PR and this seems on-point.

@nox
Copy link
Contributor Author

nox commented Mar 23, 2022

@seanmonstar Ping on this?

@seanmonstar
Copy link
Member

I think including this info in the error somehow is a good goal. We should do it. I want to figure out how exactly that info should be exposed, though. As part of 1.0, the pooling Client will likely get moved out and broken up, with most pieces probably in hyper-util. So, how do we expose this info?

  • We could have a unique error type coming from the pooling client, which is essentially enum SendError<E> { Connect(E), Http(hyper::Error) }. In that case, the info could just be part of the generic "connect" error.
  • Or we could reuse hyper::Error, having the connect variant be internal somehow. In that case, how do we attach this useful extra information to the hyper::Error? I could imagine we want to attach other useful things in the future, so what mechanism should that use?

@nox
Copy link
Contributor Author

nox commented Mar 30, 2022

IMO for now 2nd way is good, as the client still lives in Hyper. Would you mind we merge this PR as is and see how we do it for 1.0 later?

@seanmonstar
Copy link
Member

So, I could just click merge, but it will most likely be different with 1.0 (and that's starting now-ish). How do you feel now?

@nox
Copy link
Contributor Author

nox commented May 27, 2022 via email

@nox
Copy link
Contributor Author

nox commented Apr 3, 2023

@seanmonstar Would you merge it if I rebase it again?

@seanmonstar
Copy link
Member

Sure, we can just merge it into 0.14, while noting that a more generic mechanism is probably better for hyper-util (in a hyper 1.0 world).

@nox nox changed the base branch from master to 0.14.x April 14, 2023 08:45
@nox
Copy link
Contributor Author

nox commented Apr 14, 2023

I rebased it on top of 0.14.x and changed the PR base branch.

@seanmonstar seanmonstar merged commit 297dc4c into 0.14.x Apr 14, 2023
@seanmonstar seanmonstar deleted the error-connect-info branch April 14, 2023 15:41
nox added a commit to hyperium/hyper-util that referenced this pull request Jul 10, 2024
seanmonstar pushed a commit to hyperium/hyper-util that referenced this pull request Jul 12, 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

Successfully merging this pull request may close these issues.

3 participants