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

Open
akshayjshah opened this issue Jul 15, 2022 · 16 comments
Open

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

akshayjshah opened this issue Jul 15, 2022 · 16 comments

Comments

@akshayjshah
Copy link

akshayjshah commented Jul 15, 2022

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
Author

akshayjshah commented Jul 15, 2022

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

seankhliao commented Jul 15, 2022

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

@akshayjshah
Copy link
Author

akshayjshah commented Jul 15, 2022

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
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

nightlyone commented Jul 20, 2022

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
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
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
Author

akshayjshah commented Aug 12, 2022

Happy to open a PR for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Proposals (old)
Likely Accept
Development

No branches or pull requests

6 participants