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

x/net/http2: afterReqBodyWriteError wrapping causes temporary errors to fail type assertion #22136

Closed
vcabbage opened this issue Oct 4, 2017 · 6 comments
Assignees

Comments

@vcabbage
Copy link
Member

@vcabbage vcabbage commented Oct 4, 2017

https://golang.org/cl/50471 wraps some errors with afterReqBodyWriteError. This wrapping causes type assertions to the net.Error interface to fail even if the underlying error fulfills the interface.

https://github.com/golang/net/blob/a04bdaca5b32abe1c069418fb7088ae607de5bd0/http2/transport.go#L397-L405

Perhaps afterReqBodyWriteError should also fullfil net.Error and delegate the methods to the underlying error.

/cc @tombergan

@dsnet

This comment has been minimized.

Copy link
Member

@dsnet dsnet commented Oct 5, 2017

\cc @neild as an example of error tagging information lost because of wrapping.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 5, 2017

All that error handling by wrapping is terrible. I kinda wish we'd just get rid of it, somehow.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 5, 2017

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Oct 5, 2017

@vcabbage, are you seeing this problem when calling http2.Transport.RoundTrip, or when calling http2.ClientConn.RoundTrip? I believe the problem should only happen in the latter case, since http2.Transport.RoundTrip should unwrap the error.

The reason we did this is because we need to tell Transport.RoundTrip whether the error happened before or after the request body was written. We could not change the signature of ClientConn.RoundTrip without breaking clients ... but I didn't remember that changing the error type effectively changes the public API due to interface types like net.Error.

We can fix this more simply by adding an internal, unexported method like the following, which would be invoked directly by RoundTrip from both Transport and ClientConn:

type roundTripError struct {
  err error  // so the underlying error is not lost
  afterReqBodyWrite bool
}

func (ClientConn*) roundTrip(req *http.Request) (*http.Response, roundTripError)
@tombergan tombergan self-assigned this Oct 5, 2017
@vcabbage

This comment has been minimized.

Copy link
Member Author

@vcabbage vcabbage commented Oct 5, 2017

@tombergan It was happening when calling http2.Transport.RoundTrip. It looks like it's possible to return the wrapped error if retry > 6 here.

I'm a little surprised that we'd be hitting that case, but it's the only explanation I can see.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 1, 2017

Change https://golang.org/cl/75252 mentions this issue: http2: remove afterReqBodyWriteError wrapper

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
There was a case where we forgot to undo this wrapper. Instead of fixing
that case, I moved the implementation of ClientConn.RoundTrip into an
unexported method that returns the same info as a bool.

Fixes golang/go#22136

Change-Id: I7e5fc467f9c26fb74b9b83f2b3b7f8882645e34c
Reviewed-on: https://go-review.googlesource.com/75252
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Nov 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.