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

use native EventSource if available #20

Closed
wants to merge 3 commits into from
Closed

use native EventSource if available #20

wants to merge 3 commits into from

Conversation

meleyal
Copy link

@meleyal meleyal commented May 14, 2013

This ensures native EventSource will be used if it's available.

As EventSource + CORS is now more widely supported, this seems like a better default.

Currently the tests just exit if native support is detected. Probably a better solution would be to (somehow) force the tests to use the polyfill version.

@meleyal
Copy link
Author

meleyal commented May 14, 2013

Now the tests always run the polyfill version.

@Yaffle
Copy link
Owner

Yaffle commented May 14, 2013

native EventSource in Firefox does not support broken TCP connections, this polyfill addes "heartbeat"-timeout, so it will reconnect periodically, when there are no messages from server; see https://bugzilla.mozilla.org/show_bug.cgi?id=444328
also some old implementation is a little buggy, you can run tests with native ES to see that;

what do you think about? should native be used for Chrome? FF?

@meleyal
Copy link
Author

meleyal commented May 14, 2013

I see, that makes it a bit more complicated. Is there any way to detect this bug without browser sniffing?

@Yaffle
Copy link
Owner

Yaffle commented May 14, 2013

seems, no

@meleyal
Copy link
Author

meleyal commented May 14, 2013

So if I've understood correctly, this means EventSource in FF will work until the connection is broken (go offline, sleep laptop, etc.), then it will not automatically reconnect?

@Yaffle
Copy link
Owner

Yaffle commented May 14, 2013

do not know about sleep laptop and "go offline", but for situations, when the system have a wi-fi, but wi-fi access point looses internet(but wi-fi is still active), the users computer may not know, that there is a problem with internet

@meleyal
Copy link
Author

meleyal commented May 15, 2013

What about something like:

return if browserSupportsEventSource() and !eventSourceIsBuggy()

browserSupportsEventSource ->
  typeof global.EventSource !== 'undefined'

eventSourceIsBuggy ->
  navigator.userAgent.toLowerCase().indexOf('firefox') > -1

@Yaffle
Copy link
Owner

Yaffle commented May 15, 2013

why do you think, that native ES is better?

@meleyal
Copy link
Author

meleyal commented May 15, 2013

I don't have enough experience to say, it just seems like the correct behavior for a polyfill to use the native version if available.

@Yaffle
Copy link
Owner

Yaffle commented May 17, 2013

i don't think it is a good idea to use browser sniffing for that.

@kellym
Copy link

kellym commented May 17, 2013

IMO, if this works in all the common browsers, I don't mind and even prefer that it overrides the default for consistency. I think the goal is eventually for EventSource to be so fully and consistently supported that it makes this code irrelevant, but we're not there yet.

@meleyal
Copy link
Author

meleyal commented May 17, 2013

Yeah, I agree with both comments. Still, it would be nice if there was a way to opt-out and use native ES if available.
Of course you could always use conditional script loading, but that's often not practical.

@arteme
Copy link

arteme commented Jul 21, 2013

Are there any native EventSource implementations that are 100% correct? It would seem as a sensible effort to use native EventSource in browsers that got it right even on the basis that it should be cheaper resource-wise to use the compiled version instead of an interpreted one...

@Yaffle
Copy link
Owner

Yaffle commented Jul 22, 2013

@Matt-Esch
Copy link

Ok, a little opinion here. I think EventSource as a spec is horrible. That said, it's pretty sweet that we can keep persistent connections open and use lightweight http streaming to receive realtime events from the server. If you never actually use the inbuilt EventSource implementation and always fall back to xhr long polling, then you might as well implement a more expressive api. If I am never going to benefit from a single open downstream connection over the lifetime of the page then I don't actually want to use this.

@Yaffle
Copy link
Owner

Yaffle commented Aug 16, 2013

@Matt-Esch, sorry, but how does it relate to that issue?
http streaming can be used with this polyfill

@Matt-Esch
Copy link

You can use http streaming, but the browser doesn't recycle the buffer when done through xhr. So you have to close the connection at some sensible point to allow the buffer to be discarded. EventSource solves this problem. It triggers events for the data received over the http connection and then discards that portion of the response, so you don't risk leaking memory. Native event source is actually a reasonable alternative to websockets for heavy downstream.

@Yaffle
Copy link
Owner

Yaffle commented Aug 16, 2013

@Matt-Esch,
you are right, this makes native ES better, but
did you hear about moz-chunked-text responseType for XMLHttpRequest - http://lists.w3.org/Archives/Public/public-webapps/2013JanMar/0569.html ?

So you have to close the connection at some sensible point to allow the buffer to be discarded

this technique should give a not bad result

@Matt-Esch
Copy link

And what options are there for chrome?

@Yaffle
Copy link
Owner

Yaffle commented Aug 17, 2013

@Matt-Esch , i do not know about Chrome, may be Streams API ( https://dvcs.w3.org/hg/streams-api/raw-file/tip/Overview.htm ).

so,
How to detect, that native EventSource is not bad?

@Matt-Esch
Copy link

Browser sniffing or direct CORS testing could be used to determine if the implementation on the current browser is acceptable.

Legacy proxy servers are known to, in certain cases, drop HTTP connections after a short timeout. To protect against such proxy servers, authors can include a comment line (one starting with a ':' character) every 15 seconds or so

Assuming that this advice is taken into account and a CORS request actually works, are there any implementations that still have bugs? I.e. are there any CORS compliant implementations that break with a 15 second server sent heartbeat? Is there something else that needs to be tested for?

@Yaffle
Copy link
Owner

Yaffle commented Aug 18, 2013

  var m = /AppleWebKit\/(\d\d\d\.\d\d)/.exec(navigator.userAgent);
  var isWebKitAfter537dot3 = m && Number(m[1]) > 537.3;

?

@Yaffle
Copy link
Owner

Yaffle commented Aug 18, 2013

@Matt-Esch,

Chrome and FireFox stops EventSource (readyState === CLOSED) in some rare cases: after clicking on <a download>, link to a resource with Content-disposition: attachment header, back/forward navigation in subframe.

And FireFox has two a little worse bugs:

https://bugzilla.mozilla.org/show_bug.cgi?id=444328 - TCP-level keep alive timer
https://bugzilla.mozilla.org/show_bug.cgi?id=831392 - Server sent events, no reconnection

@Matt-Esch
Copy link

Both of the firefox issues should be taken care of by generic application level keepalive logic that should be running for all implementations (reconnect manually if the heartbeat dies). There are many ways to disrupt the connection and the only robust way of knowing if it's working is to have your application receive a message from the server. Having said that, The first issue is certainly much more difficult to workaround. I'm curious to know if this happens even when the EventSource code is running in its own iframe? Might be worth testing.

@Yaffle
Copy link
Owner

Yaffle commented Aug 18, 2013

Both of the firefox issues should be taken care of by generic application level keepalive logic that should be running for all implementations (reconnect manually if the heartbeat dies). There are many ways to disrupt the connection and the only robust way of knowing if it's working is to have your application receive a message from the server.

But according to information from https://bugzilla.mozilla.org/show_bug.cgi?id=444328#c1 - Chrome uses 45 seconds TCP keep-alive, so it should detect disconnects(although, only after some seconds);
and i think, that keepalive logic integreated into EventSource is a nice thing.
This polyfill uses "heartbeatTimeout" as a workaround.

The first issue is certainly much more difficult to workaround
which one did you mean by "first" ?

@Matt-Esch
Copy link

Given the recommendation by w3c to send empty comments every 15 seconds from the server, reconnection issues are far less of a problem if there is some logic to listen for these comments and to reconnect when they stop making it through. But agreed, it would be nice if the reconnection logic in EventSource just worked.

And by "first" I meant the disconnect on rare occasions problem such as downloading attachments. I'm not sure how to go about preventing certain actions from closing the connection prematurely.

@Yaffle
Copy link
Owner

Yaffle commented Aug 18, 2013

@Matt-Esch , for connection closing when "downloading attachments" this polyfill just does the reconnection
(it listens for "error" and "abort" events on XMLHttpRequest);
it is a little wrong, because, if the user wants to stop any activity, this code will do the reconnection.

The best way - to fix browsers implementation.
Although, that issue may be very rare...

@Yaffle
Copy link
Owner

Yaffle commented Dec 8, 2014

Fixed

@Yaffle Yaffle closed this Dec 8, 2014
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

5 participants