Allow to set custom websocket factory #24552
Conversation
src/lib/libsockfs.js
Outdated
| @@ -172,6 +172,19 @@ addToLibrary({ | |||
| addr = result[1]; | |||
| port = parseInt(result[2], 10); | |||
| } | |||
| } else if (typeof SOCKFS.websocketArgs["factory"] === "function") { | |||
There was a problem hiding this comment.
Wrap this whole block in #if expectToReceiveOnModule('websocket')
| ws.binaryType = 'arraybuffer'; | ||
| } catch (e) { | ||
| throw new FS.ErrnoError({{{ cDefs.EHOSTUNREACH }}}); | ||
| } |
There was a problem hiding this comment.
Would it be enough to just replace WebSocketConstructor below?
Then the rest of the code in that block could be shared too?
Or do you want to be able all the logic to do with url and subprotocol below?
Perhaps you could give more details about why / when these might be needed? Perhaps via and example and/or a test?
There was a problem hiding this comment.
I think when I define ws factory, then I know url/subprotocol/etc
Main goal is to have option to configure server ws proxy with for example ws connection url searchQuery. Or wrap ws object into Proxy for additional data filtering/management.
There was a problem hiding this comment.
OK, perhaps you can add a motivating example usage in the form of a test.
Are we sure this cannot be achieved without modifying emscripten by monkey-patching the WebSocket global perhaps?
There was a problem hiding this comment.
Not sure a test will test anything related to sockets functionality. The change is aimed for post-emscripten module integration.
Real life example: I have an app which performs ping of hosts(udp), list of hosts is fetched inside module with simple tcp connection. Using this factory I'm able to use single server ws proxy written in nodejs. Also right now I wrap ws instance's send and onmessage in proxy for additional logging, but it also can be used for any type of data manipulation.
There was a problem hiding this comment.
Well, we certainly need some kind of test for this new functionality.
How about adding a test that shows that you can inject logging this way?
@juj what do you think of this change? Can / should we leave this up to normal monkey patching mechanisms or should we provide this hook?
There was a problem hiding this comment.
To me this looks ok. It would indeed allow runtime resolution of websocket connections, as opposed to needing to know at compile time how to connect.
Definitely would be great to have a test in sockets for this functionality, to ensure that the hook/dispatch mechanism is kept functional.
Are we sure this cannot be achieved without modifying emscripten by monkey-patching the WebSocket global perhaps?
Patching WebSocket global is a bit tricky because it could then leak to other Emscripten WebSocket backends, altering their functionality.
There was a problem hiding this comment.
TBH I'm not sure I can add proper test. Maybe you can advice some existing test, which I can alter/modify?
| family: sock.family, | ||
| protocol: sock.protocol | ||
| }); | ||
| ws.binaryType = 'arraybuffer'; |
There was a problem hiding this comment.
Is there a reason that this side of code is overwriting the .binaryType? Could the factory side do it before returning?
There was a problem hiding this comment.
emscripten/src/lib/libsockfs.js
Line 226 in bf5d63c
I guess it's required for TextEncoder/Decoder and logic in recv shim
In some cases, especially when application uses more then one connection, it will be handy to define custom websocket factory.