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

proposal: net/http: add ResponseWriterAs function #39558

Closed
carlmjohnson opened this issue Jun 12, 2020 · 5 comments
Closed

proposal: net/http: add ResponseWriterAs function #39558

carlmjohnson opened this issue Jun 12, 2020 · 5 comments
Labels
Projects
Milestone

Comments

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Jun 12, 2020

Wrapping an http.ResponseWriter is difficult because handlers are supposed to test for capabilities by testing for the presence of an optional interface. This means a wrapper type cannot just have a Push method that delegates to the ResponseWriter it wraps. This does not work because the presence of the method is the only mechanism for detecting support for http.Pusher. Instead there need to be 2^N types that a wrapper function chooses from depending on which optional interfaces (Pusher, Hijacker, Flusher) its wrapped type supports. This becomes impractical very quickly.

I propose that the http package gains a function that works like errors.As. http.ResponseWriterAs(w http.ResponseWriter, target interface{}) bool will first test if w can be converted to the target type, and if not, check whether w has an Unwrap() http.ResponseWriter method then test whether any ResponseWriter in the chain can be converted.

It could also, like errors.As, test for As(target interface{}) bool methods in the chain, although I suspect this would be less useful than in the errors case.

@gopherbot gopherbot added this to the Proposal milestone Jun 12, 2020
@gopherbot gopherbot added the Proposal label Jun 12, 2020
@ianlancetaylor ianlancetaylor changed the title proposal: Add http.ResponseWriterAs() function proposal: net/http: add ResponseWriterAs function Jun 12, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 12, 2020

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 12, 2020
@rsc rsc moved this from Incoming to Active in Proposals Jun 17, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 24, 2020

We've basically stopped adding these other methods precisely for the reason you identify.

Errors have the nice property that the API is very simple and wrappers really change very little.
That's not true of http.ResponseWriter.

Suppose you had an http.ResponseWriter wrapper that did gzipping of data passed to Write.
Suppose it implemented Unwrap so that it could be hijacked.
But it chose not to implement WriteString.
If you used As(w, &writeStringer) to look for a WriteString further down the chain, you'd end up bypassing the gzipping accidentally.

It's also unclear that adding As at this point would help much, since we can't rewrite all the code in the wild that doesn't use it, nor can we rewrite all the code in the wild that doesn't implement Unwrap, and now we'd have two ways to do it (only one of which is guaranteed to work - the old way).

We are aware of the problem, though, and if/when we get to designing a second version of the http server API we should definitely think hard about how to repeat this mistake.

But I don't see how this specific proposal improves the situation given the large amount of existing code.

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Jun 24, 2020

I don't think that the .As approach would work with io.Reader/Writer though, for the reason you outlined. The optional interfaces there end up changing the semantics in ways that don't lend themselves to wrapping. I still think it might work for some of the ResponseWriter cases though.

I know Brad Fitz started looking at making a v2 of the http client. I hadn't heard anything about a v2 for http server.

I guess the question is what strategy makes more sense: a hard v2 with no backwards compatibility or a continuing evolution of what exists. I think this could be made entirely backwards compatible or at least mostly compatible with only some optional interfaces accidentally by passed.

For ResponseWriter, it might make more sense to have http.ResponseWriterIs(w, enum) bool so that there would be a way of adding a method but separately signaling whether you actually support it by checking your wrapped ResponseWriter. It could first see if w has Is(enum) bool and if so, try that, if not just check for the optional method as is done now. Code that doesn't implement the Is method will continue to work, but new code can explicitly delegate. The enum would be something like int8 that specifies the handful of optional interfaces that make sense to wrap, as opposed to Errors.As, where there is a potential infinity of optional interfaces.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 8, 2020

I guess the question is what strategy makes more sense: a hard v2 with no backwards compatibility or a continuing evolution of what exists.

But as I noted in my comment above, adding ResponseWriterAs isn't backwards compatible. Code would still have to deal with all the special case methods that exist today - they're not going away and can't be shoehorned into a different API.

Maybe when it's time to add the next such method on ResponseWriter we should talk more about this proposal, but right now it doesn't seem like it simplifies anything.

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Jul 13, 2020

It wasn't clear to me before why ResponseWriterAs wasn't backwards compatible, since the examples were around io.Writer, but I see now that the problem is that old middleware would be confused by new ones.

Basically, there are two ways to do ResponseWriterAs. One, you can have new middleware just implement all the interfaces and then use the As to delegate which ones are actually useable. This would confuse old middleware into thinking they could use them when sometimes they couldn't. Two you can implement nothing and use As to go up the Unwrap chain (as is done in errors.As), but then old middleware would think that the interfaces weren't useable even when they are.

I think two is not the worst failure case, but I concede that it's not really backwards compatible.

Closing the issue. In the event that someone proposes a new optional interface for ResponseWriter, I think something like ResponseWriterAs should be created for that new optional interface, but I don't think that's on the horizon at the moment.

@rsc rsc moved this from Active to Declined in Proposals Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants