emscripten-core
Fix emscripten_websocket_deinitialize()
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.js trying to understand how it works and figured that emscripten_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 e1c39bc59, when the javascript array of websockets was replaced by an HandleAllocator.
All references to WS.sockets were correctly replaced, save for the ones in emscripten_websocket_deinitialize() ; introducing the bug.
This pull requests consists of 3 commits.
The first one extends the test_websocket_send test to cover the use of emscripten_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 to HandleAllocator in order to avoid iterating over the allocated array directly.
I ran the following command to test the code and it outputs "OK".
test/runner sockets.test_websocket_send* --browser "C:\Program Files\Google\Chrome\Application\chrome.exe"
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.c after 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:
Fix emscripten_websocket_deinitialize()
Some references to the javascript array `WS.sockets` were left out when
the array was replaced by a `HandleAllocator` in e1c39bc59, hence breaking
the function.
The bug is fixed by iterating over the valid ids in the `HandleAllocator`
and calling `emscripten_websocket_delete()` on them to delete the websockets.
The `test_websocket_send` is extended to cover the function and highlight the
differences between using `emscripten_websocket_close()` + `emscripten_websocket_delete()`
or `emscripten_websocket_deinitialize()`.
Fixes #24613
Please let me know if there is anything wrong with the code. Thank you for your time.
Best regards, Vincent.
cc @sbc100 for the regressing commit https://github.com/emscripten-core/emscripten/commit/e1c39bc59132e5f77130d96b4ce5c5b9d71f203a
Actually, I am not using emscripten_websocket_deinitialize(), nor do I intend to. I just figured it was likely not working by looking at the code and thought that this was something I could fix.
Having a call to emscripten_websocket_delete() for each emscripten_websocket_new() is in my opinion the right thing to do; rather than relying on some global state used by the deinitialize function.
Reading the documentation however, it seems emscripten_websocket_deinitialize() was introduced to perform some cleanup operations when a thread exits. A function deleting all sockets makes sense in that context.
That being said, I wasn't able to find any such use in the codebase.
There seem to be some discrepancies, as relates to thread support, between what is advertised in the documentation versus what is actually implemented. There are several TODO in libwebsocket.js which suggest that creating a WebSocket on other than the main thread is in fact not supported. Notably, the value of createOnMainThread doesn't seem to be used at all.
But my use case is the most simple : a single websocket, and only a main thread; so I'm not sure and none of these (potential) issues is bothering me.
I won't have time to work on the branch until at least this weekend, but I will try to make the changes as soon as possible.