Add -sNODENET backend for real outgoing TCP via node:net#27080
Add -sNODENET backend for real outgoing TCP via node:net#27080guybedford wants to merge 1 commit into
Conversation
Adds a new NODENET setting that backs the POSIX sockets API directly with Node.js's node:net module, giving real, non-blocking outgoing (client) TCP sockets without WebSockets, an external proxy process, or pthreads. Unlike PROXY_POSIX_SOCKETS this is single-threaded and event-driven: socket readiness is delivered through the same emscripten_set_socket_*_callback hooks the default WebSocket backend uses, so it drops into existing readiness reactors unchanged. This initial backend supports outgoing TCP only: connect, send, recv and close, plus get/setsockopt (SO_ERROR, TCP_NODELAY, SO_KEEPALIVE and the TCP keep-alive tunables). There is no bind/listen/accept (server) support and no UDP yet; those land in follow-ups. - new nodenet_sock_ops in libsockfs.js implementing the sock_ops contract over net.createConnection - route get/setsockopt through the backend under -sNODENET, and compile out the weak __syscall_setsockopt stub via a libstubs variation so the JS symbol wins - test/sockets/test_nodenet.c: outgoing connect/send/recv against a loopback echo server started by the test harness
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Nice! I've not yet reviewed the meat of libsockfs.js but looks good so far.
| // [link] | ||
| var PROXY_POSIX_SOCKETS = false; | ||
|
|
||
| // If 1, the POSIX sockets API is backed by Node.js's ``node:net`` module, |
|
|
||
| // If 1, the POSIX sockets API is backed by Node.js's ``node:net`` module, | ||
| // giving real non-blocking outgoing TCP sockets with no WebSockets, proxy | ||
| // process or pthreads. This only works under node and is ignored elsewhere. |
There was a problem hiding this comment.
You should mention something about how this is similar to what NODERAWFS does for filesystem access?
Speaking of which, should we perhaps combine them? Or at least have NODERAWFS automatically enable NODENET. Its hard to imagine wanting one without that other maybe?
There was a problem hiding this comment.
Also, regarding the name of this settings, should we use the word SOCK rather then NET since that seems to be what we have done previously. I guess -sNODERAWSOCKETS is kind of a mouthfull.
Maybe putting it behind the existing NODERAWFS avoids having to name something new ? :)
|
|
||
| @classmethod | ||
| def get_default_variation(cls, **kwargs): | ||
| return super().get_default_variation(nodenet=settings.NODENET, **kwargs) |
There was a problem hiding this comment.
Its a little unfortunate that we need to crate this new variant here.
I wonder if we should perhaps instead move __syscall_setsockopt into JS (even when its stub). The downside is that the folks who call the stub version needless call out the JS, but the upside is simplicity in the number of variations of this native library.
Folks we really care about the cost of the stub __syscall_setsockopt can just stop calling it (or implement their own stub).
| finally: | ||
| server.shutdown() | ||
| server.server_close() | ||
| thread.join() |
There was a problem hiding this comment.
I wonder if this should go in test_sockets.py? I'm not sure..
| if data: | ||
| self.request.sendall(data) | ||
|
|
||
| server = socketserver.TCPServer(('127.0.0.1', 0), EchoHandler) |
There was a problem hiding this comment.
In test_sockets.py we preallocate specific ports, but I guess this maybe even better than doing that?
| * found in the LICENSE file. | ||
| */ | ||
|
|
||
| // Outgoing TCP over the NODENET backend. We connect to a loopback echo server |
There was a problem hiding this comment.
This test is not really specific to NODENET, right? Maybe just call it test_tcp_echo.c?
| @@ -0,0 +1,120 @@ | |||
| /* | |||
| * Copyright 2024 The Emscripten Authors. All rights reserved. | |||
| static int client_fd = -1; | ||
| static struct sockaddr_in dest; | ||
| static int connected = 0; | ||
| static int ping_sent = 0; |
There was a problem hiding this comment.
Use bool (stdbool.h) here for these state variables.
| static int connected = 0; | ||
| static int ping_sent = 0; | ||
|
|
||
| static void finish(int result) { |
There was a problem hiding this comment.
Would it be simpler just to remove all these static qualifiers? I imagine we are very inconsistent across our test code.
| return 0; | ||
| } | ||
| }, | ||
| #endif |
There was a problem hiding this comment.
Would to be easy to split this out into a new file? libnode_sockfs.js or libsockfs_node.js
2 participants
This adds a new
-sNODENETsetting that for supporting direct full sockets on Node.js via thenode:netmodule, without needingws, an external proxy process, or pthreads.It is certainly possible to extend this to UDP and incoming connections, down to some API quirks, but for now this is kept very minimal. If there is interest in a more comprehensive PR that covers the full surface area for sockets, I'm open to assisting with that further as well as a follow-on.
For comprehensive
setsockoptssupport I just posted nodejs/node#63825 as well so we can close the loop on all options being supported and that is effectively used in a backwards compatible way here despite not landing yet.Perhaps in future we can consider alternative backends like https://sockets-api.proposal.wintertc.org/, but since that API is not yet supported on Node.js, for now this approach can cover Emscripten outbound connections for both Node.js and Cloudflare Workers.
Note: AI was used to create this PR, under my review.