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.

Sign up for GitHub

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

Jump to bottom

net/http/httptrace: WroteHeaders should be called after flush or provide an option to do so #60576

Open
domdom82 opened this issue Jun 2, 2023 · 4 comments
Open

net/http/httptrace: WroteHeaders should be called after flush or provide an option to do so #60576

domdom82 opened this issue Jun 2, 2023 · 4 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@domdom82
Copy link

domdom82 commented Jun 2, 2023

Where we are coming from

We have a setup where an L4 proxy (Envoy) is between the go client and the target server. When the client opens a connection to the server, the proxy first accepts the TCP connection and does a TLS handshake at the frontend before trying to proxy the request forward to the backend.

If the backend is dead, the proxy will immediately close the frontend connection again. This results in a broken pipe at the client, because it was just about to send HTTP data and will now receive an io.EOF error (actually, it is a nothingWrittenError unwrapped to an EOF but I digress..)

The issue we need to solve

Now the issue for us is, we would like to know if we can safely retry the request or not. For GET/HEAD/OPTIONS etc. that's easy.
However, for POST/PUT requests we need to know if the server has actually received anything (= don't retry) or not (= retry).
Since all we get is an EOF, we can't be sure.

httptrace

In an attempt to mitigate the situation we turned to httptrace and its GotConn and WroteHeaders callbacks.
The idea was to use them as indicators if any part of the request has actually made it to the backend server or not. Now, the problem with WroteHeaders is that it is called when the request headers have been written to the bufio.Writer NOT the wire.

For us, this is pretty useless because of course a write to an in-memory buffer always succeeds. It does not tell you anything about whether or not the headers were actually sent to the target server. I question the usefulness of that callback.

I noticed that further down the headers are actually flushed if the request is a POST with a body and some other quirky conditions (I'll get to that later).

My suggestion is to move the call to trace.WroteHeaders() below the (potential) flush to have some safety that the server has actually seen the headers before we claim that we wrote them.

An alternative could be the addition of a new FlushedHeaders callback in httptrace that is specific to this use case and is put here, i.e. called when no error occurred during flushing.

Now about those 'quirky' flushing conditions...

I've read into the commit that introduced this behavior and wondered why it was done this way. Like the comment suggests, we should flush "just in case the server needs the headers early" - which is the case for us. However, we would like to be able to flush on all requests, including e.g. POSTs without a body. So can't we just make FlushHeaders a configurable field of Request and pass it down to the transferWriter? This way the client can decide if it wants to send early headers to the server, instead of having this decision taken away by the SDK.

tl;dr

  • httptrace.WroteHeaders is called already even if nothing was written to the wire
  • Either: Move it below Flush()
  • Or: Add a httptrace.FlushedHeaders callback that is called after Flush()
  • Make FlushHeaders an option for requests so that the client can decide if their want to send headers early or not
@mknyszek mknyszek changed the title http.Request: httptrace.WroteHeaders should be called after flush or provide an option to do so net/http/httptrace: WroteHeaders should be called after flush or provide an option to do so Jun 2, 2023
@mknyszek mknyszek added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest and removed Proposal labels Jun 2, 2023
@mknyszek mknyszek added this to the Backlog milestone Jun 2, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Jun 2, 2023

CC @neild

@neild
Copy link
Contributor

neild commented Jun 2, 2023

We could move ClientTrace.WroteHeaders to after the flush, but I don't think it would help this case.

The HTTP/2 client calls WroteHeaders after flushing, but it does so unconditionally regardless of whether the write has succeeded: https://go.googlesource.com/net/+/refs/tags/v0.10.0/http2/transport.go#1513

Currently, a trace always follows a sequence of WroteHeaderField calls with WroteHeaders, even if writing the headers fails. I don't think we can change that.

In addition, calling WroteHeaders after the header Flush call in the HTTP/1 path would still happen before headers are written in the case where the headers are not flushed prior to writing the body. Moving the call to after the body write begins might be possible, but starts getting very complicated.

Perhaps a better approach would be to add a distinguishable error condition to RoundTrip that can be used to determine whether a request was written. Something along the lines of:

resp, err := client.Do(req)
if errors.Is(err, http.ErrRequestNotWritten) {
  // this request failed before being written
} else if err != nil {
  // this request may have been sent prior to failure
}

However, we would like to be able to flush on all requests, including e.g. POSTs without a body.

The flush in this case writes the headers before starting to write the request body, to ensure that headers go out if the Request.Body io.Reader blocks without providing data. If there's no body, there's no concept of sending headers before the body; the headers are the entire request.

I could understand wanting a way to disable the header flush, to let headers go out in the same TCP packet as the start of the body, but I don't know why you'd want to add flushes that aren't performed now.

@maxmoehl
Copy link

maxmoehl commented Jun 4, 2023 •
edited

If the nothingWrittenError could be exposed that would solve our issue since we could rely on that for retries. But our assumption was that changing / wrapping the error is not possible since it will break if err == io.EOF {} statements? I faintly remember that there was an issue for that somewhere but can't find it right now.

Edit: @domdom82 and I are working together and discussed this issue before opening it.

@domdom82
Copy link
Author

domdom82 commented Jun 12, 2023 •
edited

It's here.
The error gets unwrapped to EOF directly, or to transportReadFromServerError which is then also unwrapped to EOF.

Sure, it would help a lot if the unwrapped error types would be returned.

Edit: @neild the unwrapping of more specific errors such as nothingWrittenError was the reason we reached for httptrace in the first place. We needed a way to distinguish a "nothing written - EOF" from a "closed while reading response - EOF". In both cases the error is EOF but we would be able to retry a "nothing written EOF". So we used httptrace as sort of side-channel to decide whether the EOF can be retried or not, by using the WroteHeaders callback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@neild @mknyszek @domdom82 @maxmoehl