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

fix: fire close on fail for WebSocket #3548

Closed
wants to merge 3 commits into from

Conversation

eXhumer
Copy link
Contributor

@eXhumer eXhumer commented Sep 4, 2024

This relates to...

Fixes #3546

Rationale

close event is never fired after a WebSocket connection is failed to establish.

Changes

  • change WebSocket.#onSocketClose when trying to access closingInfo. WebSocket.#parser is undefined when connection fails to establish.
  • run WebSocket.#onSocketClose after firing error event, which in turn fires the close event.

Features

N/A

Bug Fixes

  • fix close event not firing on failed connection.

Breaking Changes and Deprecations

N/A

Status

* see #3546

Signed-off-by: eXhumer <exhumer@exhumer.cc>
@marximimus
Copy link

LGTM

Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

please add a test

@eXhumer
Copy link
Contributor Author

eXhumer commented Sep 5, 2024

Some of the autobahn test keep failing locally due to tests timing out. If there is any actual issue unrelated to timeout, please let me know.

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

I think the test can test a little bit more

* updated with requested changes

Signed-off-by: eXhumer <exhumer@exhumer.cc>
Copy link
Contributor

@Uzlopak Uzlopak 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
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

a bunch of autobahn tests are failing

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 5, 2024

We can remove line 525, where we set the connection status, because we call #onSocketClose.

And the change in #onSocketClose with optional chaining brakes the autobahn. Because it changes the error code to 1006 instead of 1005.

Probably need to make #onSocketClose accept a parameter. If we pass a symbol kFail than we handle the onFail case and code stays at 1005. Or maybe use the wasClean variable?

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

.

@KhafraDev
Copy link
Member

The issue is that onSocketClose is not appropriate to call here. You need to handle setting the state to CLOSED and emitting a close event in a separate function that both onFail and onSocketClose can use.

@eXhumer
Copy link
Contributor Author

eXhumer commented Sep 5, 2024

Sorry for not getting to back to this earlier. After KhafkaDev said there were autobahn tests failing, I wanted to properly setup the tests locally to make sure they were working as expected. Although the test seem to be running fine on CI, it seems to fail test cases between 12.4.3 and 12.4.18 locally, while passing on CI. Does anyone have any insight on why this may be the case? And in some cases, re-running the same test locally seem to produce different fails. I have tried to do the autobahn test on two different machines (macOS 15.1 & Windows 11), both seem to fail these 16 tests locally.

│ 12.4.3  │ 'FAILED'        │ 'UNCLEAN'       │ 399      │ null            │
│ 12.4.4  │ 'FAILED'        │ 'UNCLEAN'       │ 135      │ null            │
│ 12.4.5  │ 'FAILED'        │ 'UNCLEAN'       │ 91       │ null            │
│ 12.4.6  │ 'FAILED'        │ 'UNCLEAN'       │ 54       │ null            │
│ 12.4.7  │ 'FAILED'        │ 'UNCLEAN'       │ 181      │ null            │
│ 12.4.8  │ 'FAILED'        │ 'UNCLEAN'       │ 351      │ null            │
│ 12.4.9  │ 'FAILED'        │ 'UNCLEAN'       │ 630      │ null            │
│ 12.4.10 │ 'FAILED'        │ 'UNCLEAN'       │ 1022     │ null            │
│ 12.4.11 │ 'FAILED'        │ 'UNCLEAN'       │ 61       │ null            │
│ 12.4.12 │ 'FAILED'        │ 'UNCLEAN'       │ 209      │ null            │
│ 12.4.13 │ 'FAILED'        │ 'UNCLEAN'       │ 419      │ null            │
│ 12.4.14 │ 'FAILED'        │ 'UNCLEAN'       │ 722      │ null            │
│ 12.4.15 │ 'FAILED'        │ 'UNCLEAN'       │ 1147     │ null            │
│ 12.4.16 │ 'FAILED'        │ 'UNCLEAN'       │ 1066     │ null            │
│ 12.4.17 │ 'FAILED'        │ 'UNCLEAN'       │ 1052     │ null            │
│ 12.4.18 │ 'FAILED'        │ 'UNCLEAN'       │ 1042     │ null            │

I was unable to do any reliable testing locally as a result.

@Uzlopak Uzlopak closed this in #3565 Sep 7, 2024
@eXhumer eXhumer deleted the fire-close-on-fail-ws branch September 8, 2024 09:53
Uzlopak pushed a commit that referenced this pull request Sep 20, 2024
* see #3546, #3548 & #3565

Signed-off-by: eXhumer <exhumer@exhumer.cc>
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.

WebSockets do not fire 'close' event if the connection failed to be established
4 participants