-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
Hijack before WriteHeader to avoid issues with middleware and Gin #335
Comments
I'd have to see your gzip middleware to be sure but I suspect the issue is that the gzip middleware is buffering But then the gzip middleware knows there was a The reason Gorilla worked is that it hijacks before writing the upgrade response and so there is no To fix, change the gzip response writer's As for why I opted to write the upgrade response with the net/http response writer, it's far simpler. Compare Gorilla's code to my library:
But perhaps it's a good idea to add a For example, the popular nytime's Go gzip response writer does not: https://github.com/nytimes/gziphandler/blob/2f8bb1d30d9d69c8e0c3714da5a9917125a87769/gzip.go#L250 |
@nhooyr This happens with unrolled/logger too if you want to take a look at that. I commonly use the following setup in a lot of my projects, and the above basically prevents me from fully migrating t oyour library server-side (have only done so client-side so far):
Of course this varies a bit from project to project but you get the idea. |
Hmmm I would have go essentially go fork a bunch of "middleware" libraries and "make them my own". I'm okay with that 👌 If this is the correct thing to do? |
It's actually surprisingly hard to find good Middleware that actualy works correctly and doesn't suck 😅 |
@prologic In the example above, if you remove the gzip handler entirely, does the issue still occur with just unrolled/logger? I know the error message seems to indicate that the error is in unrolled/logger but if my understanding is correct, it's a red herring. |
Yes. It occurs with both. I've already done the necessary pulling bits out to see wtf was going on 😅
I think it's just because of the order of the wrapping handlers? (middleware) |
Hmm, I'm not sure how the error could arise with just unrolled/logger. Can you add a |
Sure I'll try and poke around and get back to you 👌 |
Friendly reminder re the above. |
I'll just change my library to hijack first like Gorilla. |
I tried to upgrade to your library in one of my projects msgbus in this commit but unfortunately ran into some issues and had to revert
It would appear that the interaction between Logging and GZIP MIddleware is not playing nicely, whereas the old
gorilla/websocket
library was handling this fine.Basic error I'm seeing is:
Can we fix this somehow so that your library plays nicely with middleware that wraps it potentially?
The text was updated successfully, but these errors were encountered: