Skip to content

experimental/websockets: support code and reason for close() and close event #4989

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

etodanik
Copy link

What?

Closes #4972

With a small caveat: I wasn't sure the proper implementation for wasClean and prefer to contribute it as a second later PR, after I get to read the RFC spec about it a bit more in depth.

Checklist

  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests for my changes.
  • I have run linter and tests locally (make check) and all pass.

@etodanik etodanik requested a review from a team as a code owner July 30, 2025 12:51
@etodanik etodanik requested review from mstoykov and inancgumus and removed request for a team July 30, 2025 12:51
@CLAassistant
Copy link

CLAassistant commented Jul 30, 2025

CLA assistant check
All committers have signed the CLA.

@etodanik
Copy link
Author

etodanik commented Aug 5, 2025

@mstoykov @inancgumus just clarifying if there's anything else I can help with, or is this good on track for release?
I see tests are failing, but they're rather flaky to begin with. They actually pass, but that whole test suite has leaks that are there regardless of this PR.

@mstoykov
Copy link
Contributor

mstoykov commented Aug 5, 2025

Hi @etodanik, thank you for the PR.

We are in the last week before releasing v1.2.0 - planned for next week. As such we are unlikely to be able to review this, within the week.

Looking at the CI - you probably should try to fix the lint issues. Also test are failing with races within the experimental websockets, while they weren't before. So you probably have introduced a race condition. Running test with -race locally might help you find the root cause and confirm that you have fixed.

I would argue both will need to be fixed before this is mergeable.

@etodanik
Copy link
Author

etodanik commented Aug 5, 2025

Hi @etodanik, thank you for the PR.

We are in the last week before releasing v1.2.0 - planned for next week. As such we are unlikely to be able to review this, within the week.

Looking at the CI - you probably should try to fix the lint issues. Also test are failing with races within the experimental websockets, while they weren't before. So you probably have introduced a race condition. Running test with -race locally might help you find the root cause and confirm that you have fixed.

I would argue both will need to be fixed before this is mergeable.

Ok. I'll attend to the linter stuff as well as the tests. However, I should note that even if I remove my tests and code completely , that whole test suite is flaky & fails half the time for me

@etodanik etodanik force-pushed the fix/ws-support-code-reason branch from 12e4a30 to 56162f7 Compare August 7, 2025 15:56
@etodanik
Copy link
Author

etodanik commented Aug 7, 2025

@mstoykov could we re-run CI/CD tasks? I fixed linter's complaints about long line & test flakiness.

@etodanik
Copy link
Author

etodanik commented Aug 7, 2025

Appreciate it. Hmm , I'll look at what the GH Action actually runs and make sure to get the same fails locally. Looks like I'm not catching the data race errors.

@etodanik
Copy link
Author

Alright, data race should be fixed now :)

@etodanik etodanik force-pushed the fix/ws-support-code-reason branch from da64c15 to eff0a6d Compare August 12, 2025 12:08
@etodanik
Copy link
Author

@mstoykov ready for another go at CI/CD :)

@etodanik etodanik force-pushed the fix/ws-support-code-reason branch from eff0a6d to 4a69c0b Compare August 13, 2025 15:45
@etodanik etodanik force-pushed the fix/ws-support-code-reason branch from 4a69c0b to df143bf Compare August 15, 2025 21:15
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: Add reason, code and wasClean fields for close event
3 participants