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.

Sign up for GitHub

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

Jump to bottom

.close() gives exception #130

Closed
ejerskov opened this issue May 21, 2019 · 11 comments
Closed

.close() gives exception #130

ejerskov opened this issue May 21, 2019 · 11 comments

Comments

@ejerskov
Copy link

Im trying to close my SSE connection but I get an exception.

I initiate like this:
this.source = new EventSourcePolyfill(url, { headers: { "X-Requested-With": "XMLHttpRequest" } });

And then I close it like this:
this.source.close();

I get the following exception:
Uncaught (in promise) DOMException

And Chrome tells me that the issue is on line 460:
reader.cancel();
@Yaffle
Copy link
Owner

Yaffle commented May 30, 2019

I cannot reproduce it, is there a way to setup a server and reproduce it?

@ejerskov
Copy link
Author

Hmm, shame. I can't copy my server side code here.
Do you have any idea why it would cause an exception in this case? It's really ruining my error log with all the errors.
Can I "swallow" the exception in some way?

@chrisckc
Copy link

I also get this when calling close() using version 1.0.5 and Chrome 74.0.3729.169 on Mac

Uncaught (in promise) DOMException

(anonymous) | @ | eventsource.js:460
-- | -- | --
  | close | @ | eventsource.js:809
  | EventSourcePolyfill.close | @ | eventsource.js:888
  | es.onmessage | @ | sse-notifications-receiver:151
  | fire | @ | eventsource.js:602
  | onProgress | @ | eventsource.js:758
  | (anonymous) | @ | eventsource.js:470

The connection is closed, it just that it causes an exception.

Seems like a client or browser issue, if this is a server issue, is there a sample server that works with this client such that close() does not cause an exception?

What server was used for the development and testing of this client?

@chrisckc
Copy link

@Yaffle I am able to reproduce the issue using the example server code in the Readme.

I created a Node server using the example server js and used the index.html also as per the example.

I had to modify the index.html to explicitly create an instance of EventSourcePolyfill instead of just EventSource as recent versions of chrome already include EventSource so the polyfill is not used. If the polyfill is not used, the close() exception does not occur.

I just added a setTimeout to close the connection after 5 seconds and boom, the Dom exception occurs.

Here is my modified index.html (i also modified the script path /src/eventsource.js in both the index.html and server.js to match my file structure)

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8" />
    <title>EventSource example</title>
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <script src="/src/eventsource.js"></script>
    <script>
      // use EventSourcePolyfill explicitly
      var es = new EventSourcePolyfill("events.php");
      var listener = function (event) {
        var div = document.createElement("div");
        var type = event.type;
        div.appendChild(document.createTextNode(type + ": " + (type === "message" ? event.data : es.url)));
        document.body.appendChild(div);
      };
      es.addEventListener("open", listener);
      es.addEventListener("message", listener);
      es.addEventListener("error", listener);
      es.addEventListener("close", listener);

      window.setTimeout(function(){ es.close(); }, 5000);
    </script>
</head>
<body>
</body>
</html>

@Yaffle
Copy link
Owner

Yaffle commented Jun 27, 2019

ok, I was able to reproduce it, thank you.
It seems,

  1. It is better not to call both of controller.abort() and reader.cancel():
        controller.abort();
        reader.cancel();
  1. It is needed to not reject the error if it is an AbortError.

Yaffle added a commit that referenced this issue Jun 27, 2019
@Yaffle
a change to catch the uncaught exception, see #130
5eb75b8
@chrisckc
Copy link

chrisckc commented Jul 2, 2019

Version 1.0.7 works great, thanks!

@ejerskov
Copy link
Author

ejerskov commented Jul 2, 2019

Thank you for solving the issue! Greatly appreciated!

@ejerskov ejerskov closed this as completed Jul 2, 2019
@earshinov
Copy link

For anyone interested, AbortError is still happening in Firefox, but imo it is now a Firefox issue.

StackBlitz: https://stackblitz.com/edit/firefox-fetch-abort-error
Firefox bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1583815

@Yaffle
Copy link
Owner

Yaffle commented Sep 27, 2019 •
edited

@earshinov ,

IMHO, it can be reproduced like this:

  var controller = new AbortController();
  var signal = controller.signal;
  fetch('data:text/plain,test', {signal}).then(function(response) {
    const reader = response.body.getReader();
    controller.abort();
    //reader.cancel(); - this needs to be used to avoid the log message, seems
  }).catch(function(e) {
    console.log('catched', e);
  }).finally(function(e) {
    console.log('done');
  });

@Yaffle
Copy link
Owner

Yaffle commented Sep 27, 2019

@earshinov, a workaround was added, but it is still throws on the page unload

@earshinov
Copy link

@Yaffle , I guess all errors can be fixed by subscribing to the Promise returned from Reader.cancel:

reader.cancel().catch(() => {});

After this change I cannot reproduce the AbortError in Firefox any longer.

I updated my StackBlitz example: https://stackblitz.com/edit/firefox-fetch-abort-error

@thijstriemstra thijstriemstra mentioned this issue Aug 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@earshinov @Yaffle @chrisckc @ejerskov