Fix emscripten_websocket_deinitialize()

[strandfield]

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 e1c39bc, 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.

[kripken]

cc @sbc100 for the regressing commit e1c39bc

[sbc100]

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?

[sbc100]

Can you flip these block around so that this is possive test (i.e. #ifdef TEST_EMSCRIPTEN_WEBSOCKET_DEINITIALIZE)

[sbc100]

Why do we need force exit here?

[strandfield]

To be honest, I am not sure how it works.
I saw that it was used above in WebSocketClose() and I interpreted that at meaning "the test is completed, exit with result code 0 (success)", so I did the same. Since my version of the test never enters WebSocketClose(), I force exit here.
As far as I understand, using emscripten_force_exit() is needed because emscripten_exit_with_live_runtime() is called in main(); otherwise the program would continue to run ?

[sbc100]

Why do we need this change here but not in the other block?

[strandfield]

In the existing version of the test, emscripten_websocket_close() is used to close individual socket and we are entering the WebSocketClose() callback; and the test ends when both sockets have been closed.

My version of the test uses emscripten_websocket_deinitialize() instead; so WebSocketClose() is never called and I need a way to know when to deinitialize.
I introduced the binary_received variable to be able to know when both sockets have received the binary message so that I can deinitialize.
Testing this variable in the other case is not necessary as they are close individually, so we know we are done when both sockets are closed.

I will try to clarify things in a new commit.

[sbc100]

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?

[strandfield]

Yes, no problem. I will just revert my last commit. Better play it safe.

[strandfield]

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.