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

proposal: net/http: add Transport.WriteTimeout attribute #61568

Open
pfreixes opened this issue Jul 25, 2023 · 8 comments
Open

proposal: net/http: add Transport.WriteTimeout attribute #61568

pfreixes opened this issue Jul 25, 2023 · 8 comments
Labels
Milestone

Comments

@pfreixes
Copy link

Today using the default implementation of the http Client provided by Golang there is no an easy way for having a write timeout which should be triggered if all writes do not succeed in some specific time.

By having this configuration at hand, mapped as a new attribute of the transport, like Transport.WriteTimeout users could give up on requests earlier, without having to wait for an overall timeout (send request, send body, read response, read response body).

This new parameter would be similar to the Transport.ResponseHeaderTimeout but giving up earlier if writes do not succeed after X time.

Why this new attribute and not leverage on the current Transport.ResponseHeaderTimeout?

Main reason is that currently the timer for triggering is not started until all writes have been made - including sending the body. If Im not mistaken here is where it happens here

And writes could take much longer or even never finish, like what could happen with a TCP connection that is in zombie state - the TCP buffer is full and no acks are received by the server.

Reuse the Conn.SetWriteDeadline for implementing the write timeout

Golang connection already provides a function that can be used for implementing this timeout Conn.SetWriteDeadline, but considering that connections can be moved back to the pool for subsequent requests the implementation should take care of calling the Conn.SetWriteDeadline with the new dead line every time that a new request comes in and reuse an existing connection

@gopherbot gopherbot added this to the Proposal milestone Jul 25, 2023
@ianlancetaylor
Copy link
Contributor

CC @neild @bradfitz

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented Aug 5, 2023

This sounds similar to http2.Transport.WriteByteTimeout added within #48830

The #61777 also mentions http1

I'd originally thought to propose adding WriteByteTimeout to HTTP/1 connections as well, but there's less motivation for it there: HTTP/1 connections only handle a single request at at time, and a connection can only be reused after a complete request/response cycle is completed. There's less need to separate the response write timeout from an individual byte-write timeout. In addition, it's pretty much impossible to implement a byte-write timeout when using the sendfile path.

Maybe it would make sense to add something like this for HTTP/1, but it's simpler to consider HTTP/2 for now.

@neild
Copy link
Contributor

neild commented Aug 7, 2023

This is a reasonable request, but tricky to implement.

I recently took a stab at implementing WriteByteTimeout for HTTP/1 client and server connections. It proved to be more difficult than anticipated, mostly because the HTTP/1 implementation can use ReadFrom or WriteTo to transfer bodies. You can't set a per-byte timeout with ReadFrom/WriteTo. It might be feasible to avoid ReadFrom/WriteTo (losing any optimizations along those paths) only when WriteByteTimeout is set. I eventually decided the complexity wasn't worth the potential benefit.

So I think this proposal seems fine in principle, but needs a CL to demonstrate implementation feasibility.

@pfreixes
Copy link
Author

pfreixes commented Aug 7, 2023

Hey @neild thanks for the feedback, notice that the proposal does not want to go at "byte" level but just provide a new deadline for having the write phase done - regardless the bytes sent.

This - I could be missing something here - should be "doable" by basically adding a new timer around here

So I think this proposal seems fine in principle, but needs a CL to demonstrate implementation feasibility.

Not sure what CL stands for, but I could provide a PR with a proposal.

Let me know!

@neild
Copy link
Contributor

neild commented Aug 7, 2023

Ah, I misunderstood the proposal. (Sorry, I read the comment referencing http2.Transport.WriteByteTImeout first and was primed to think of this as same feature for HTTP/1.)

You can set a timeout covering the entire request using NewRequestWithContext. This isn't precisely what you're describing, because it covers the entire request from sending the request through reading the response, but it avoids the problem with ResponseHeaderTimeout not starting until after the request is sent.

Do you have a need for separate timeouts for sending and receiving, or is a single timeout covering the full request sufficient? If you do need separate timeouts, can you expand a bit more on why?

@pfreixes
Copy link
Author

pfreixes commented Aug 8, 2023

Do you have a need for separate timeouts for sending and receiving, or is a single timeout covering the full request sufficient? If you do need separate timeouts, can you expand a bit more on why?

From my understanding a request for HTTP 1.X protocol has three very differentiated phases and all of them might have a different timeout requirement:

1 - Sending the request to the server + the full body
2 - Having the request processed by the server
3 - Having the request processed by the server and returned all response body

Today the net/http package provides support for 2 and 3, but not by 1. The addition of a new configuration for setting the timeout for the write phase would allow the users to specify a timeout - that could be totally different to the ones used by 2 and 3 - used only for the write phase.

While an overall request timeout could be fine to be for example 300 seconds, if the write did not succeed in less than 30s might not make any sense to have to wait the overall request time. By having this new timeout configuration you can start a retry strategy earlier without having to wait the overall request time.

IMO this follows what we have Today with the ResponseHeaderTimeout, which does not consider the cost of transferring the response body to the client. The proposal for the WriteTimeout would be semantically close since will consider only the cost of sending the body to the server.

One of the benefits that also comes with WriteTimeout is that it enables the possibility of detecting broken connections faster. Today if the TCP connection is broken and it has not ben ack by the client - this happens from time to time since not always RST or FIN packages are guaranteed to be delivered, the request might just get block during the writes since the TCP buffer might get full - no ones is acking in the other side. With the write timeout, the caller would not need to wait until the overall request timeout.

@pfreixes
Copy link
Author

@neild any thoughts on this one?

@neild
Copy link
Contributor

neild commented Aug 21, 2023

1 - Sending the request to the server + the full body
2 - Having the request processed by the server
3 - Having the request processed by the server and returned all response body

This is not quite correct: It is possible for the server to start responding before reading the request body, and it is possible for writes to the request body and reads from the response body to be interleaved. (See also #57786.)

I think the sequence for a request is:

  1. Send request headers.
  2. Optional: If the request has an "Expect: 100-continue" header, start ExpectContinueTimeout timer.
  3. Send request body.
  4. Start ResponseHeaderTimeout timer.

The response may be read at any time after step 1, potentially even before the ResponseHeaderTimeout timer begins.

The request context applies a timeout/cancelation for the overall request/response sequence.

It does look to me like there's a place for an additional timer covering the time before the ResponseHeaderTimeout timer is armed. In the case of a request with no body, this timer would cover the entire request write. What about a request with a body, however? A POST can be arbitrarily large, and the timeout for a 1K body shouldn't be the same as for a 1G one. Do we need separate timers covering writing the request headers and the request body? Do we need a timer for the request body at all, or is the context's timeout sufficient?

Perhaps a good start would be to add Transport.WriteHeaderTimeout, setting the amount of time allowed to write request headers. For requests with no body (most GET requests), this is equivalent to a full request write timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

5 participants