-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
reverseproxy: ignore RFC 1521 parameters in mime type when checking the Content-Type header #3758
Conversation
Without this change, a Content-Type header like "text/event-stream;charset=utf-8" would not trigger the immediate flushing.
Thanks for the PR! I will look at this soon. ... and I'm kinda beginning to wonder if -1 should be the default for everything at this point. Thoughts, @francislavoie / @mohammed90 ? |
Idk, that sounds like too risky a change to me 🤔 |
Why? |
Isn't there risk that we break behaviour for some clients? There must've been a good reason for it not to be set to -1 by default in stdlib, no? The code comes from here https://golang.org/src/net/http/httputil/reverseproxy.go?h=FlushInterval#L410 Should we bubble this change upstream to see what they have to say? I know our proxy implementation is not dependent on it but it feels like we should let them know of fixes like this? |
We don't know if it is likely to break anyone; the worst I imagine could happen would be a performance degradation. (I mean, I'm sure it could break someone but I doubt it'd be the majority case.) Anyway, that's tangential, just thought I'd bring it up...
Sure. Sounds like a good idea. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix @cmfcmf !
I have now opened an upstream issue about this fix: golang/go#47359 |
Without this change, a Content-Type header like "text/event-stream;charset=utf-8"
would not trigger the immediate flushing.
Fixes #3765