Skip to content

Allow to set custom websocket factory #24552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Allow to set custom websocket factory #24552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
396c825
Allow to set custom websocket factory
ololoken Jun 12, 2025
b6e661c
wrap ws factory with expectToReceiveOnModule(websocket)
ololoken Jun 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions site/source/docs/porting/networking.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ This is the default build mode for Emscripten. Use the linker flag
to connect to, and the linker flag ``-sWEBSOCKET_SUBPROTOCOL`` or
``Module['websocket']['subprotocol']`` to control the connection type
(``'binary'`` or ``'text'``).
Or define custom WebSocket factory using ``Module['websocket']['factory']``. Factory
is called with options param ``{addr, port, type, family, protocol}`` and
must return WebSocket instance.

Full POSIX Sockets over WebSocket Proxy Server
==============================================
Expand Down
19 changes: 18 additions & 1 deletion src/lib/libsockfs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,24 @@ addToLibrary({
addr = result[1];
port = parseInt(result[2], 10);
}
} else {
}
#if expectToReceiveOnModule('websocket')
else if (typeof SOCKFS.websocketArgs["factory"] === "function") {
try {
ws = SOCKFS.websocketArgs["factory"]({
addr,
port,
type: sock.type,
family: sock.family,
protocol: sock.protocol
});
ws.binaryType = 'arraybuffer';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason that this side of code is overwriting the .binaryType? Could the factory side do it before returning?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emscripten/src/lib/libsockfs.js

Line 226 in bf5d63c

ws.binaryType = 'arraybuffer';

I guess it's required for TextEncoder/Decoder and logic in recv shim

} catch (e) {
throw new FS.ErrnoError({{{ cDefs.EHOSTUNREACH }}});
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I'm not sure I can add proper test. Maybe you can advice some existing test, which I can alter/modify?

}
#endif
else {
// create the actual websocket object and connect
try {
// The default value is 'ws://' the replace is needed because the compiler replaces '//' comments with '#'
Expand Down