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

feat(client): add SendRequest::try_send_request() method #3691

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

seanmonstar
Copy link
Member

This change adds a try_send_request() method to SendRequest. This method returns a TrySendError type, which allows for returning the request back to the caller if an error occured between queuing and trying to write the request.

This method is added for both http1 and http2.

Closes #3673
Closes seanmonstar/reqwest#2325

(Well, after hyper-util makes use of it.)


Problem

If you try to send_request(req) at the same time the server is closing the connection, it's possible hyper will notice the HUP before the request gets serialized. If so, theoretically the request is safe to try on another connection.

But there isn't a mechanism for hyper to give the request back. And since the pool was moved out of hyper, it doesn't know how to just grab another connection.

Need: a mechanism to return the request on specific kinds of errors.

Options

  • Something like try_send_request(req), which returns a TrySendError.
  • Embed the request into the hyper::Error.
  • Put some channel-like thing in req.extensions().

TrySendError

let mut req = Request::new(body);
loop {
    match tx.try_send_request(req).await {
        Ok(_) => yay,
        Err(TrySendError::Queued { error, request }) => {
            req = req_back;
            continue;
        },
        Err(TrySendError::Sent { error }) => {
            return Err(e);
        }
    }
}

Pros:

  • From a Rust perspective, it's simple. There's std::sync::mpsc::TrySendError as prior art.
  • Cheap, no allocations needed, even on the happy path.

Cons:

  • Would be needed for each http1::SendRequest and http2::SendRequest.
  • Intangibly, it feels "clumsy". Why wouldn't send_request() have handled this?
    • If after trying this out and finding it should be the way, we could create fn send() and hide try_send_request() and send_request().
  • Structural matching and enums must be designed with future-compatility changes.
    • We don't have to expose an enum. It can just have methods.

Embed in hyper::Error

After writing all this out, this is probably not possible.

Pros:

  • Feels more elegant.

Cons:

  • How do you access the request after?
    • Adding Error::request() set of APIs seems to pollute the type.
      • Perhaps an ext API can access it...
        • But that would require allocation even on success.
  • What if you keep the Error around but didn't expect the Request<B> to live with it?
    • Is this really a problem? Unknown... But if it is...
    • Could an ext be set to signal to hyper that it's OK? Then it becomes opt-in.
  • Cannot embed Request<B> if the body is !Send
    • Can we even do this generically at all?
    • Maybe if the ext passed converts into an Any...

Pass ext::CaptureTrySend?

let mut req = Request::new(body);
loop {
    // inserts a capture channel extension, returns the receiver
    let try_rx = hyper::ext::capture_try_send(&mut req);

    match tx.send_request(req).await {
        Ok(_) => yay,
        Err(e) => {
            // take it back
            if let Some(req_back) = try_rx.req().await {
                // try again...
                req = req_back;
                continue;
            } else {
                return Err(e);
            }
        }
    }
}

Pros:

  • Works with any B
  • Doesn't keep request stuck in the Error for longer than expected

Cons:

  • Requires allocating req.extensions() for basically every request...

This method returns a `TrySendError` type, which allows for returning
the request back to the caller if an error occured between queuing and
trying to write the request.

This method is added for both `http1` and `http2`.
@seanmonstar seanmonstar merged commit 4ffaad5 into master Jul 1, 2024
21 checks passed
@seanmonstar seanmonstar deleted the client-try-send branch July 1, 2024 13:19
bartlomieju added a commit to denoland/deno that referenced this pull request Jul 2, 2024
Reland of #24056 that doesn't
suffer from the problem that was discovered in
#24261.

It uses upgraded `hyper` and `hyper-util` that fixed the previous
problem in hyperium/hyper#3691.
sbmsr pushed a commit to sbmsr/deno-1 that referenced this pull request Jul 2, 2024
Reland of denoland#24056 that doesn't
suffer from the problem that was discovered in
denoland#24261.

It uses upgraded `hyper` and `hyper-util` that fixed the previous
problem in hyperium/hyper#3691.
zebreus pushed a commit to zebreus/deno that referenced this pull request Jul 8, 2024
Reland of denoland#24056 that doesn't
suffer from the problem that was discovered in
denoland#24261.

It uses upgraded `hyper` and `hyper-util` that fixed the previous
problem in hyperium/hyper#3691.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant