Fix emscripten_websocket_deinitialize() #25744
Conversation
The test currently fails as emscripten_websocket_deinitialize() is currently broken.
Since it is possible to build such a list by manually iterating over the `allocated` array, I think it is better to provide a function that does just that. Accessing the `allocated` member from outside the class should be avoided as it is more an implementation detail than a part of the class interface.
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Thanks for tracking down the issue and fixing this.
And thanks for the details PR description.
I wasn't even aware of emscripten_websocket_deinitialize previously. Out of interest why are you using that over closing individual sockets?
| } | ||
| printf("\n"); | ||
|
|
||
| #ifndef TEST_EMSCRIPTEN_WEBSOCKET_DEINITIALIZE |
There was a problem hiding this comment.
Can you flip these block around so that this is possive test (i.e. #ifdef TEST_EMSCRIPTEN_WEBSOCKET_DEINITIALIZE)
| assert(result == EMSCRIPTEN_RESULT_INVALID_TARGET); | ||
| (void)ready_state; | ||
| sock1 = sock2 = 0; | ||
| emscripten_force_exit(0); |
There was a problem hiding this comment.
Why do we need force exit here?
| // https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/readyState | ||
| assert(ready_state == 2); // 2 = CLOSING | ||
| #else | ||
| if (binary_received == 2) { |
There was a problem hiding this comment.
Why do we need this change here but not in the other block?
| } | ||
| } | ||
| return valid_ids; | ||
| } |
There was a problem hiding this comment.
I don't think we want to add more method to this class I'm afraid because it shared with all the other users.
Maybe just implement this locally in the websocket code?
3 participants
Hello,
First of all, thank you for making this great piece of technology.
Running my C++ in a web browser feels overpowered and is really fun.
I was looking at
libwebsocket.jstrying to understand how it works and figured thatemscripten_websocket_deinitialize()likely does not work as it references an undefined variable (WS.sockets).It seems another user came to the same conclusion (issue #24613).
As far as I can tell, this bug was introduced in commit e1c39bc, when the javascript array of websockets was replaced by an
HandleAllocator.All references to
WS.socketswere correctly replaced, save for the ones inemscripten_websocket_deinitialize(); introducing the bug.This pull requests consists of 3 commits.
The first one extends the
test_websocket_sendtest to cover the use ofemscripten_websocket_deinitialize()and fails with the current code.The second commit ought to fix the bug and make the test pass.
The last commit is more of a suggestion than anything else, it adds a
list()method toHandleAllocatorin order to avoid iterating over theallocatedarray directly.I ran the following command to test the code and it outputs "OK".
There are many test cases I couldn't get to work, even on the main branch ; but that is probably a skill issue on my part.
I hope the command I ran is sufficient.
I also tried to run clang-format on
test_websocket_send.cafter modifying the file but it produced so many diffs I rolled back the changes. This file doesn't seem to adhere to the style as defined in.clang-format(very long lines, right-aligned pointers) so I left it that way.I think the following may be used as commit message if the branch is squashed:
Please let me know if there is anything wrong with the code.
Thank you for your time.
Best regards,
Vincent.