-
Notifications
You must be signed in to change notification settings - Fork 55
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
Disable upgrade to xhr_streaming if cloudflare headers detected #342
Conversation
function actOnConnectHeaders(headers) { | ||
if(headers && headers.server && (headers.server.indexOf('cloudflare') > -1)) { | ||
/* Cloudflare doesn't support xhr streaming */ | ||
XHRStreamingTransport.isAvailable = function() { return false; }; |
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.
You're comfortable that this is set globally, not just in this library instance? Will it ever get re-enabled? Don't we expect that the restriction should only apply to fallback hosts?
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.
Don't we expect that the restriction should only apply to fallback hosts?
Do we? I'm not sure how we're planning on using cloudflare -- mightn't we want to enable it on all endpoints in some circumstances (eg DDOS attack)?
You're comfortable that this is set globally, not just in this library instance?
Are we anticipating that people might have multiple instances of the library connecting to different endpoints?
(If so, there are other parts of the library which don't really work well with multiple instances, e.g. the log level and handler are set globally. If we think there are valid usecases for multiple realtime instances, we should fix those too.
We might, but my point is that once you have detected it on one host it doesn't then mean you want that restriction, forever, on all hosts.
I'm more concerned about the possibility that it never gets re-enabled, than I am about the issue of multiple instances sharing state. |
bf0f0e4
to
406c475
Compare
Fair enough. Though given you have to reinitialize the lib if you want to change host, making it per-instance solves that, right? (ie no need to also make it re-enablable for a given lib instance) |
But a single library instance might be required to connect to a fallback, and then later be able to use the normal host. You don't think we should try to make streaming work again in that case? |
81f7c28
to
5005a70
Compare
Right, yes, of course. Sorry, was being stupid. 5005a70 |
5005a70
to
8dcfb40
Compare
That would work, but TBH I quite like that at the moment the connectionManager instance doesn't know anything about the current host (it's in the transportParams instance which is created by startConnect and passed around to the things that need it). So I'd rather not set properties on connectionManager that depend on what the current host being passed around; this seems cleaner even if it's a couple lines more code |
No description provided.