Add Emscripten WebSockets API #7670

juj · 2018-12-14T09:59:32Z

Adds Emscripten WebSockets API: direct bare WebSockets support exposed to C code in multithreading aware manner. A C integer handle represents the WebSocket connection, and it can be shared across threads. Still slightly experimental, a number of TODOs left, but may be useful to others.

(One TODO in particular is backproxying - currently WebSocket events will all be dispatched on the main browser thread, and not the requested thread)

kripken

A few early comments.

What do you think about writing new libraries like these in a more SDL2 manner, that is, in C (or C++), and use EM_ASM/EM_JS for the necessary JS parts?

kripken · 2018-12-18T21:24:44Z

src/library_websocket.js

+  emscripten_websocket_get_url_length__proxy: 'sync',
+  emscripten_websocket_get_url_length__sig: 'iii',
+  emscripten_websocket_get_url_length: function(socketId, urlLength) {
+    var socket = WS.sockets[socketId];


could have a WS.get(id) helper that contains the debug ifdef (could also pass the function name in there, optionally).

That would regress on release mode code size, because the if (!socket) return {{{ cDefine('EMSCRIPTEN_RESULT_INVALID_TARGET') }}}; part would still need to be in the caller side. So while it would reduce duplication of the #if WEBSOCKET_DEBUG part, it would make the code larger by creating a redundant function. (var socket = WS.sockets[socketId]; minifies probably to exact same length as var socket = WS.get(socketId);, but the function approach would have an extra WS: { get: function(socketId) { return this.sockets[socketId]; } } part to it.

Ideal solution to avoid code duplication here would be to use macros:

#if WEBSOCKET_DEBUG #define WS_GET(socketId, name) WS.sockets[socketId]; \ if (!socket) { \ console.error(name + '(): Invalid socket ID ' + socketId + ' specified!'); \ return {{{ cDefine('EMSCRIPTEN_RESULT_INVALID_TARGET') }}}; \ } #else #define WS_GET(socketId, name) WS.sockets[socketId]; \ if (!socket) return {{{ cDefine('EMSCRIPTEN_RESULT_INVALID_TARGET') }}}; #endif and then in all parts, use `var socket = WS_GET(socketId);`. (But that would require support for full C preprocessing)

kripken · 2018-12-18T21:25:13Z

src/library_websocket.js

+
+  emscripten_websocket_get_url_length__proxy: 'sync',
+  emscripten_websocket_get_url_length__sig: 'iii',
+  emscripten_websocket_get_url_length: function(socketId, urlLength) {


urlLength is not used anywhere?

Good catch, I'll update that.

juj · 2018-12-19T15:13:20Z

What do you think about writing new libraries like these in a more SDL2 manner, that is, in C (or C++), and use EM_ASM/EM_JS for the necessary JS parts?

Using EM_ASM/EM_JS is a bad practice that generates poor code compared to JS libraries. I definitely do like implementing as much as possible in C/C++ code, but none of these functions lend well to being implemented in C/C++ code, because they access the WebSocket object that is JavaScript state. Note how the other PR #7672 builds on top of this in a way that #7672 does not have any JS code in it.

kripken · 2018-12-19T19:54:07Z

What do you mean by poor code? It's true that EM_ASMs are looked up by an integer id, so metadce/closure/etc. can't optimize out unused ones. But EM_JS emits a proper function for each one, which is fully optimizable. Is there something else poor there?

For the library here, though, yeah, maybe it's fine either way, since as you said it's almost all JS API glue code. I wonder though if more wouldn't be needed eventually even here?

juj · 2019-01-07T17:01:41Z

What do you mean by poor code? It's true that EM_ASMs are looked up by an integer id, so metadce/closure/etc. can't optimize out unused ones.

Yeah, that is what I referred to here. The array lookup is worse compared to having each function written out in full.

But EM_JS emits a proper function for each one, which is fully optimizable. Is there something else poor there?

EM_JS() has a limitation that it cannot proxy, so it will need proxying support added to it first. I admit I have not looked at the codegen of EM_JS() that close if there are other differences. One drawback is that the JS code would then live in a .c or a .cpp file, which would throw off IDEs completely, the current .js library files with some #ifs confuse IDEs a little bit less and give mostly working syntax highlighting at least.

juj · 2019-01-07T17:47:45Z

Updated to add test to sockets suite.

juj · 2019-01-11T12:55:26Z

This is now ready for a second review pass to merge?

juj · 2019-01-15T13:49:11Z

Ping @kripken, I wish this would be able to merge soon, it is becoming a bit urgent.

kripken · 2019-01-15T17:55:45Z

Got it, reading now.

EM_JS() has a limitation that it cannot proxy, so it will need proxying support added to it first.

Hmm, they do expand into JS library functions, so the proxying support we have there could work, if they were marked as such - seems like that could be done.

kripken

lgtm with some minor comments.

We don't run websockets tests on CI - does the new test pass for you? We should really get at least a subset of them running on CI - like maybe even just starting with your new test here, if it's robust enough (many of the old ones are not, sadly), please look into that later when you can.

kripken · 2019-01-15T17:57:57Z

src/settings.js

@@ -361,6 +361,9 @@ var WEBSOCKET_SUBPROTOCOL = 'binary';
 // Print out debugging information from our OpenAL implementation.
 var OPENAL_DEBUG = 0;

+// If 1, prints out debugging related to WebSockets calls. If 2, traces bytes communicated via the socket.
+var WEBSOCKET_DEBUG = 0;


this only works with the new library, correct, and not the old sockets code? may be worth documenting that here.

Check, adjusted the comment to refer to emscripten/websocket.h.

kripken · 2019-01-15T17:58:48Z

system/include/emscripten/websocket.h

+  const char *url;
+  const char **protocols;
+
+  // If true, the created socket will reside on the main browser thread. If false, the created socket is bound to the calling thread.


these docs should be moved to the emscripten.h area under site/. Can leave for a followup though, not urgent.

juj added multithreading networking labels Dec 14, 2018

juj mentioned this pull request Dec 14, 2018

full_posix_sockets #7672

Merged

kripken reviewed Dec 18, 2018

View reviewed changes

juj force-pushed the emscripten_websockets branch 2 times, most recently from 39c7364 to 0baccda Compare January 7, 2019 17:45

juj force-pushed the emscripten_websockets branch 2 times, most recently from cf89066 to aa421c2 Compare January 10, 2019 10:16

kripken approved these changes Jan 15, 2019

View reviewed changes

Add Emscripten WebSockets API

1b9bbe7

juj force-pushed the emscripten_websockets branch from aa421c2 to 1b9bbe7 Compare January 16, 2019 20:21

juj merged commit ac00e06 into emscripten-core:incoming Jan 16, 2019

merlinzerbe mentioned this pull request Feb 11, 2019

compiling and running tests/websocket/websocket.c gives missing function #8061

Closed

aliasdevelopment mentioned this pull request May 5, 2019

pthread sigmask/atfork no-op when using NNG #8545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Emscripten WebSockets API #7670

Add Emscripten WebSockets API #7670

juj commented Dec 14, 2018

kripken left a comment

kripken Dec 18, 2018

juj Jan 7, 2019

kripken Dec 18, 2018

juj Dec 19, 2018

juj commented Dec 19, 2018

kripken commented Dec 19, 2018

juj commented Jan 7, 2019

juj commented Jan 7, 2019

juj commented Jan 11, 2019

juj commented Jan 15, 2019

kripken commented Jan 15, 2019

kripken left a comment

kripken Jan 15, 2019

juj Jan 16, 2019

kripken Jan 15, 2019

juj Jan 16, 2019

Add Emscripten WebSockets API #7670

Add Emscripten WebSockets API #7670

Conversation

juj commented Dec 14, 2018

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juj commented Dec 19, 2018

kripken commented Dec 19, 2018

juj commented Jan 7, 2019

juj commented Jan 7, 2019

juj commented Jan 11, 2019

juj commented Jan 15, 2019

kripken commented Jan 15, 2019

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment