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

Always handle web socket open when connecting #1522

Merged
merged 2 commits into from Aug 16, 2022

Conversation

WilliamBergamin
Copy link
Contributor

Summary

This should resolve #1521
An edge case seem to be present when the finite state machine (fns) is not in the authenticated state and the listeners are initialized. This can happen when the application was connected (initialized), lost connection and is reconnecting.

If for some reason the web socket open event is triggered before authentication is successful the fns falls-back to handling this event at the connecting state level instead of the submachine. This causes an fns error which terminates the running process.

This pr changes the behavior of the fns to ignore web socket open events when it is in the connecting state and the submachine is not authenticated, this should prevent the process from terminating.

Requirements (place an x in each [ ])

@WilliamBergamin WilliamBergamin added the bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented label Aug 15, 2022
@WilliamBergamin WilliamBergamin self-assigned this Aug 15, 2022
@WilliamBergamin
Copy link
Contributor Author

I've tested these changes locally by allowing certain variables to be public but not sure how we could add unit tests without refactoring part of the package

@seratch seratch added this to the socket-mode@1.3.2 milestone Aug 15, 2022
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks for fixing. This should be fine. As you mentioned, writing tests for this module is still hard. It's okay to skip adding tests this time.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

LGTM! Left a tiny comment to comment the change to help us in the future 😄

packages/socket-mode/src/SocketModeClient.ts Show resolved Hide resolved
@WilliamBergamin WilliamBergamin merged commit 27b0a5e into main Aug 16, 2022
@WilliamBergamin WilliamBergamin deleted the handle_socket_open_in_connecting branch August 16, 2022 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

State machine: Unhandled event 'websocket open' in state 'connecting'.
3 participants