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

Emit "error" and "end" events on WebsocketProvider close #3486

Merged
merged 2 commits into from
Apr 27, 2020
Merged

Conversation

cgewecke
Copy link
Collaborator

@cgewecke cgewecke commented Apr 26, 2020

Description

Adds "error" and "end" event emissions to the Provider "close" logic, restoring behavior that existed in the WebsocketProvider prior to 1.2.7.

At the moment if you've attached these listeners directly to the provider, they're not firing as expected when the client drops.

For background, this topic was raised in the #3190 review here. @nivida noted:

The old WS provider did have an error listener which triggered the same error as the close listener and because it was cleaning up the response callback array in the provider on errorwasn't it triggering the error a second time when the close listener got triggered. Another difference is that the RequestManager wasn't listening to provider errors before. One of these provider-specific and not request specific errors is the MaxAttemptsReachedOnReconnectingError on line 412.

Believe the change in this PR addresses that concern. The subscription error callbacks are fired and deleted in the close handler before the final error event is emitted. Have added test logic in 4a3ef1e to verify that subscription error handlers are only run once when the connection drops.

Fixes #3485

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have tested my code on the live network.

@cgewecke cgewecke added 1.x 1.0 related issues Bug Addressing a bug Review Needed Maintainer(s) need to review labels Apr 26, 2020
Copy link

@hfa0 hfa0 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

Looks great, and really nice tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Bug Addressing a bug Review Needed Maintainer(s) need to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Websocket Provider "error" and "end" events not fired when client disconnects
3 participants