experimental/websockets: support code and reason for close() and close event #4989
Conversation
etodanik
requested review from
inancgumus and
mstoykov
and removed request for
a team
|
|
|
@mstoykov @inancgumus just clarifying if there's anything else I can help with, or is this good on track for release? |
|
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 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
force-pushed
the
fix/ws-support-code-reason
branch
from
12e4a30 to
56162f7
Compare
|
@mstoykov could we re-run CI/CD tasks? I fixed linter's complaints about long line & test flakiness. |
|
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. |
|
Alright, data race should be fixed now :) |
etodanik
force-pushed
the
fix/ws-support-code-reason
branch
from
da64c15 to
eff0a6d
Compare
|
@mstoykov ready for another go at CI/CD :) |
etodanik
force-pushed
the
fix/ws-support-code-reason
branch
from
eff0a6d to
4a69c0b
Compare
etodanik
force-pushed
the
fix/ws-support-code-reason
branch
from
4a69c0b to
df143bf
Compare
|
Rebased to |
| defaultCloseHandler := conn.CloseHandler() | ||
| conn.SetCloseHandler(func(code int, text string) error { | ||
| close(closeReceived) | ||
| once.Do(func() { close(closeReceived) }) |
There was a problem hiding this comment.
Is this really necessary? I would expect we would've hit this over the years? Or is it just that this isn't used by anyone actually?
| rt := w.vu.Runtime() | ||
|
|
||
| if code != 0 && !isValidClientCloseCode(code) { | ||
| common.Throw(rt, fmt.Errorf( | ||
| "InvalidAccessError: Failed to execute 'close' on 'WebSocket': "+ | ||
| "The close code must be either 1000, or between 3000 and 4999. %d is neither", | ||
| code, | ||
| )) | ||
| } | ||
|
|
||
| if !isValidCloseReason(reason) { | ||
| common.Throw(rt, errors.New( | ||
| `SyntaxError: Failed to execute 'close' on 'WebSocket': The message must not be greater than 123 bytes`, | ||
| )) | ||
| } | ||
|
|
There was a problem hiding this comment.
You can save yourself some trouble, by changing close to return an error and just returning it here without calling throw. It is used in two places, in the one you need to propagate the error in the other (the mapping ot the WebSocket Object) Sobek will do so for your
| w.closeCode = code | ||
| w.closeReason = reason | ||
| } | ||
| _ = w.conn.Close() |
There was a problem hiding this comment.
Not certain why originally I didn't close this hsere, but request it happens elsewhere, but not certain this needs to be in this PR.
Did you have problems without this?
Ah, sorry, didn't catch the notification. I see it now! Will answer in the coming day or two ! |
|
Hi @etodanik it has been a month. I am going to close this PR now, and if you find the time to finish it, we will gladly look at new PR or you can comment here and I will reopen it. Thank you for all the work so far 🙇 |
|
Hey, would love to give it a push next week! Can we reopen? Would be a shame to not include it, since it's also sort of a deal breaker for us internally with k6. I'll look at it again & push it past the edge of what needs to be fixed.
|
4 participants
What?
Closes #4972
With a small caveat: I wasn't sure the proper implementation for
wasCleanand 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
make check) and all pass.