-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
Comments
This sounds similar to http2.Transport.WriteByteTimeout added within #48830 The #61777 also mentions http1
|
This is a reasonable request, but tricky to implement. I recently took a stab at implementing So I think this proposal seems fine in principle, but needs a CL to demonstrate implementation feasibility. |
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
Not sure what CL stands for, but I could provide a PR with a proposal. Let me know! |
Ah, I misunderstood the proposal. (Sorry, I read the comment referencing You can set a timeout covering the entire request using 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 Today the 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 One of the benefits that also comes with |
@neild any thoughts on this one? |
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:
The response may be read at any time after step 1, potentially even before the 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 Perhaps a good start would be to add |
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 connectionThe text was updated successfully, but these errors were encountered: