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

nethttp: implement http.Hijacker in response wrapper #25

Closed

Conversation

kujenga
Copy link

@kujenga kujenga commented Jul 16, 2018

This is needed in order for users of this package to implement websocket
support, as without it, the statusCodeTracker struct does not implement
the correct interface because it knows nothing about the underlying
implementation of it's embedded http.ResponseWriter.

For reference, many other project have had this same issue, here's a
list of some found in a quick search for the issue, I'm sure there are
others as well:

This is needed in order for users of this package to implement websocket
support, as without it, the statusCodeTracker struct does not implement
the correct interface because it knows nothing about the underlying
implementation of it's embedded http.ResponseWriter.

For reference, many other project have had this same issue, here's a
list of some found in a quick search for the issue, I'm sure there are
others as well:
- golang/go#14797
- census-instrumentation/opencensus-go#642
- go-chi/chi#131
- nytimes/gziphandler#26
- caddyserver/caddy#133
- emicklei/go-restful#354
@yurishkuro
Copy link
Collaborator

please see #11

@kujenga
Copy link
Author

kujenga commented Jul 17, 2018

Ah! Thank you for pointing that out, I'd missed that. @yurishkuro do you plan to move forward with merging that PR? Or should I leave this in place as a possible alternative?

@yurishkuro
Copy link
Collaborator

yurishkuro commented Jul 17, 2018

As far as I can tell there are issues with both PRs. The previous one is too heavy from perf point of view. Yours is functionally incorrect because it makes the object always look like it's a Hijacker, even when the underlying object wasn't (in other words it may cause unexpected behavior in the applications that do something special in case of a Hijacker).

Ideally someone should implement something similar to httpsnoop, but without the additional overhead. I haven't looked at httpsnoop in a while, maybe it was refactored to separate the interfaces support from other functionally.

@kujenga
Copy link
Author

kujenga commented Jul 18, 2018

It's a fair point! I took this approach because it's the same approach that all the projects I linked to in the description took when implementing their fixes.

I can think on other fixes though, the first thing that comes to mind is having multiple structs which are used for the different purposes and implement different sets of interfaces.

@yurishkuro
Copy link
Collaborator

I suggest looking at httpsnoop implementation, it does use different structs, but because the number of their permutations is pretty large I think it auto-generates the code for them.

@kujenga
Copy link
Author

kujenga commented Oct 9, 2018

@yurishkuro what are your thoughts on the best way to move forward with this functionality? Shall I close this PR out in favor of another? It sounds like #26 is the most in line with what you are looking for.

It would be great to have this issue resolved.

@yurishkuro
Copy link
Collaborator

I think #26 is a better approach

@kujenga
Copy link
Author

kujenga commented Oct 18, 2018

Closing this in favor of #26

@kujenga kujenga closed this Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants