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

Stop re-creating EventSources upon receiving a close event #772

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Apr 7, 2022

Context: #567. In that PR, the SDK was updated to handle the event: close event as well as recreate the EventSource object when the event occurred to keep the stream open.

However, according to the MDN documentation on server-side events:

By default, if the connection between the client and server closes, the connection is restarted. The connection is terminated with the .close() method.

Now, it's unclear if the above became spec'd behavior after #567 or not, but the point is, it appears we no longer need to recreate the event stream. Doing so causes test failures such as this one:

  1) integration tests: streaming
       handles close message:
     Error: done() called multiple times
      at IncomingMessage.<anonymous> (test/integration/streaming_test.js:51:9)
      at IncomingMessage.emit (node:events:526:28)
      at emitCloseNT (node:internal/streams/destroy:138:10)
      at processTicksAndRejections (node:internal/process/task_queues:82:21)

This is likely because the call builder is recreating the event stream on the close, making it close twice.


(Adding reviewers from #567.)

@Shaptic Shaptic added the bug label Apr 7, 2022
@Shaptic Shaptic requested review from tamirms, acharb and a team April 7, 2022 19:52
@Shaptic Shaptic self-assigned this Apr 7, 2022
Copy link

@acharb acharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!
seems odd that the error just shows up in the test now though, @Shaptic any idea why that is? (or is it an irregular error that pops up occasionally)

@Shaptic
Copy link
Contributor Author

Shaptic commented Apr 11, 2022

@acharb I've only seen it in browser tests when using Node 16 (and then, only semi-consistently, I think...) 🕵️ but now it's happening consistently since Node 16 is in the test matrix...

@Shaptic Shaptic merged commit 9f57a3c into master Apr 11, 2022
@Shaptic Shaptic deleted the stop-reopening-sse branch April 11, 2022 21:17
Shaptic added a commit that referenced this pull request May 16, 2022
Shaptic added a commit that referenced this pull request May 17, 2022
* Add test which fails w/o the change
* Revert "Stop re-creating EventSource connections on close events (#772)"
This reverts commit 9f57a3c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants