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

net/http: easier access to HTTP/2 error codes #53896

Closed
akshayjshah opened this issue Jul 15, 2022 · 19 comments
Closed

net/http: easier access to HTTP/2 error codes #53896

akshayjshah opened this issue Jul 15, 2022 · 19 comments

Comments

@akshayjshah
Copy link
Contributor

x/net/http2 exports the StreamError type. Because this type is exported, it's easy to use errors.As to extract the HTTP/2 error code (if any) from any error. Unfortunately, the StreamError type gets unexported when x/net/http2 is vendored into net/http. Since we can't use errors.As, extracting HTTP/2 error codes requires some brittle string parsing. Now that we've all gotten used to post-1.13 errors, this feels bad. 😿

It would be nice if net/http exposed HTTP/2 error codes more cleanly. A simple approach might be to export an alias from net/http:

type HTTP2StreamError = http2StreamError

This is concise and requires minimal coordination with x/net/http2. However, it leaves the http2ErrCode type unexported, along with the error code constants. An easier-to-use but more elaborate approach might be to define new StreamError and ErrCode types, along with an As method on http2StreamError:

type ErrCode uint32

const (
  ErrCodeNo = ErrCode(http2ErrCodeNo)
  ...similarly for other codes...
)

type StreamError struct {
  StreamID uint32
  Code ErrCode
  cause error
}

func (e *StreamError) Error() string { ... }

func (e *StreamError) Unwrap() error { 
  return e.cause
}

func (e *http2StreamError) As(target any) bool {
  if se, ok := target.(*StreamError); ok {
    *se = StreamError{
      StreamID: e.StreamID,
      Code: ErrCode(e.Code),
      cause: e.Cause,
    }
    return true
  }
  if e.Cause == nil {
    return false
  }
  return errors.As(e.Cause, target)
}

Is there any appetite for the latter approach?

@neild
Copy link
Contributor

neild commented Jul 15, 2022

What's the use case for exposing the specific error code? HTTP/2 error codes are mostly variations on "you did something wrong" (PROTOCOL_ERROR, etc.) or "I'm broken" (INTERNAL_ERROR). I'd be interested in which cases warrant distinguishing between individual errors. Perhaps we should provide some higher-level distinguishable error describing broken streams rather than specific HTTP/2 codes.

Is there a case for exposing the stream ID? This is usually entirely opaque to the user.

@akshayjshah
Copy link
Contributor Author

What's the use case for exposing the specific error code?

As part of https://github.com/bufbuild/connect-go, I'm implementing this portion of the gRPC HTTP/2 specification. Most HTTP/2 error codes get mapped to gRPC's INTERNAL status code, but some get mapped to UNKNOWN, UNAVAILABLE, CANCELLED, RESOURCE_EXHAUSTED, or PERMISSION_DENIED. The gRPC HTTP/2 protocol is uncommonly picky about low-level transport details, but in general it's nice for RPC packages built on net/http to be able to reflect the semantics of HTTP/2 errors in their own error taxonomy.

I could imagine using a higher-level error to distinguish between these cases rather than exposing the HTTP/2 errors directly, but the new error seems like it would be fairly complex in its own right. Since they're so well-specified, perhaps it's easier to expose the HTTP/2 errors?

Is there a case for exposing the stream ID? This is usually entirely opaque to the user.

Not that I can imagine. I included them because they're already exported by x/net/http2, but we could always add them to net/http later if they prove necessary.

@seankhliao
Copy link
Member

does using http2.ConfigureServer(httpServer, h2Server) expose the http2 errors?

@akshayjshah
Copy link
Contributor Author

does using http2.ConfigureServer(httpServer, h2Server) expose the http2 errors?

I'm primarily interested in the client side, so I'm not sure.

@seankhliao
Copy link
Member

seankhliao commented Jul 15, 2022

try http2.ConfigureTransports then?

@neild
Copy link
Contributor

neild commented Jul 15, 2022

I don't really want to copy all the HTTP/2 error code constants into net/http. Without doing so, we can't move http2.StreamError into net/http, however.

We could introduce a more limited "net/http".HTTP2StreamError type and have http2.StreamError convert to it.

But I think that it's reasonable to say that if you need access to specific HTTP/2 error codes, you should use the golang.org/x/net/http2 package. As @seankhliao suggests, you can use http2.ConfigureTransports function to use the x/net implementation rather than the bundled one. The net/http package docs already suggest this approach when access to lower-level HTTP/2 features is required.

@akshayjshah
Copy link
Contributor Author

akshayjshah commented Jul 15, 2022

I don't really want to copy all the HTTP/2 error code constants into net/http. Without doing so, we can't move http2.StreamError into net/http, however.

It seems okay to leave the codes as uint32. The meaning of each code is part of the HTTP/2 spec rather than being Go-specific, so the constants are just a small convenience. That said, I'm sympathetic to your desire to keep net/http as high-level as possible, especially with HTTP/3 on the horizon.

But I think that it's reasonable to say that if you need access to specific HTTP/2 error codes, you should use the golang.org/x/net/http2 package.

If I were the owner of the main package, I'd happily use x/net/http2. Unfortunately, I'm writing a standalone RPC package, so the user brings the HTTP client. I don't see a way for me to use http2.ConfigureTransports that reliably handles client-side middleware, which typically wraps http.RoundTripper. Is there a good approach that you're aware of?

Separately, I'm reluctant to depend on x/net/http2 because it's part of a large and unstable module. As far as I'm aware, the Go team's typical advice is that stable modules should not depend on unstable modules. Even if breaking changes in x/net/http2 are unlikely to affect my package's APIs, I'd rather not have updates to my modfile force my users to pull in all the breaking changes to the whole x/net module.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jul 20, 2022
@rsc
Copy link
Contributor

rsc commented Jul 20, 2022

We are talking about ways to eliminate the difference between the vendored http2 and x/net/http2 when both are linked in. If we did that, you could import x/net/http2 and those types would be exactly the ones for errors.As. We should probably wait to see how that all shakes out.

In the interim, if the bundled copies implemented an As method that used reflect to recognize the http2 copies and translate themselves over, then client code could use x/net/http2 and the bundled copy would say "yes, I can give you an error like those" and do the translation. Does anyone want to check and see how much work that would be? (It has to use reflect because the bundled copy can't import x/net/http2 directly.)

@rsc rsc moved this from Incoming to Active in Proposals (old) Jul 20, 2022
@rsc
Copy link
Contributor

rsc commented Jul 20, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@nightlyone
Copy link
Contributor

Another approach might be to move those errors to golang.org/x/net/http/httpguts or similar. That would allow clean sharing without reflection hacks.

@akshayjshah
Copy link
Contributor Author

akshayjshah commented Jul 22, 2022

We are talking about ways to eliminate the difference between the vendored http2 and x/net/http2 when both are linked in. If we did that, you could import x/net/http2 and those types would be exactly the ones for errors.As. We should probably wait to see how that all shakes out.

This would be very welcome! Once this happens, would x/net/http2 implicitly be covered by the Go 1 compatibility promise (or is that TBD)?

In the interim, if the bundled copies implemented an As method that used reflect to recognize the http2 copies and translate themselves over, then client code could use x/net/http2 and the bundled copy would say "yes, I can give you an error like those" and do the translation. Does anyone want to check and see how much work that would be? (It has to use reflect because the bundled copy can't import x/net/http2 directly.)

This is a nice middle ground. I'm happy to take a look.

@akshayjshah
Copy link
Contributor Author

akshayjshah commented Jul 23, 2022

This seems to do the trick:

package http

import "reflect"

func (e http2StreamError) As(target any) bool {
	val := reflect.ValueOf(target)
	if val.Kind() != reflect.Pointer {
		return false
	}
	val = val.Elem()
	if t := val.Type(); t.PkgPath() != "golang.org/x/net/http2" || t.Name() != "StreamError" {
		return false
	}
	val.FieldByName("StreamID").SetUint(uint64(e.StreamID))
	code := val.FieldByName("Code")
	code.Set(reflect.ValueOf(e.Code).Convert(code.Type()))
	cause := val.FieldByName("Cause")
	if e.Cause != nil {
		cause.Set(reflect.ValueOf(e.Cause))
	} else if !cause.IsNil() {
		cause.Set(reflect.Zero(cause.Type()))
	}
	return true
}

func (e http2StreamError) Unwrap() error {
	return e.Cause
}

This implementation passes some cursory tests, though I wouldn't be surprised to find out that I should be using reflect differently. If the proposal is approved, I'm happy to open a CL.

@rsc
Copy link
Contributor

rsc commented Jul 27, 2022

@akshayjshah Thanks for prototyping that. This seems like a good path forward to me. It will "just work" as if net/http were really using golang.org/x/net/http, and one day if it really is using that, the behavior will be the same. All with no new API.

Does anyone object to making net/http implement As testing for x/net/http's errors? @neild?

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Aug 3, 2022
@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc changed the title proposal: net/http: easier access to HTTP/2 error codes net/http: easier access to HTTP/2 error codes Aug 12, 2022
@rsc
Copy link
Contributor

rsc commented Aug 12, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc modified the milestones: Proposal, Backlog Aug 12, 2022
@akshayjshah
Copy link
Contributor Author

Happy to open a PR for this!

@neild
Copy link
Contributor

neild commented Aug 15, 2022

Feel free to send me a CL, and I can review it. Thanks!

@gopherbot
Copy link

Change https://go.dev/cl/425104 mentions this issue: net/http: add errors.As support for x/net/http2.StreamError

@gopherbot
Copy link

Change https://go.dev/cl/450515 mentions this issue: doc/go1.20: add release notes for net/http and net/http/httputil

gopherbot pushed a commit that referenced this issue Nov 15, 2022
For #41773
For #41773
For #50465
For #51914
For #53002
For #53896
For #53960
For #54136
For #54299

Change-Id: I729d5eafc1940d5706f980882a08ece1f69bb42c
Reviewed-on: https://go-review.googlesource.com/c/go/+/450515
Auto-Submit: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@golang golang locked and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Proposals (old)
Likely Accept
Development

No branches or pull requests

6 participants