Skip to content

Implement built-in node:net impl - #2217

Closed
jasnell wants to merge 4 commits into
mainfrom
jsnell/node-net
Closed

Implement built-in node:net impl#2217
jasnell wants to merge 4 commits into
mainfrom
jsnell/node-net

Conversation

@jasnell

@jasnell jasnell commented Jun 5, 2024

Copy link
Copy Markdown
Collaborator

Implement a subset of the node:net and node:tls modules supporting net.Socket, tls.TLSSocket, net.BlockList, and net.SocketAddress. The net.Server and tls.TLSServer types are explicitly unsupported and won't be implemented.

Update: The key next step on this is implementing tests... Folks should feel free to start performing code review on the main piece. If folks would like a walkthrough to help with the code review, let me know

Very rough todo list:

  • Get it basically working ... ;-) ... basic implementation of the core functions. Difficult to delineate all the specific tasks involved here.
    • net.connect(...) creates the Socket, normalizes the arguments, and forwards the args to socket.connect(...)
    • socket.connect(...) validates the input arguments, initiates the underlying socket connection
    • new Socket([options]) works
      • options.allowHalfOpen works and passes through to the underlying Socket
      • options.fd throws an error if set
      • options.readable is ignored since options.fd is unsupported
      • options.writable is ignored since options.fd in unsupported
      • options.signal sets an AbortSignal that destroys the net.Socket when triggered
      • Setting options.handle to an existing standard Socket instance should wrap that socket Will implement this separately as it's not critical for immediate compat in the short term
    • Initialize and establish the connection
    • connect and ready events are emitted when the connection is opened
    • close event is emitted when destroy is called
    • close event hadError argument is true when destroy is called with an error
    • error event is emitted when destroy is called with an error
    • destroy is called with an error when attempt to establish a connection fails
    • connectionAttempt event is emitted when initializing a connection
    • connectionAttemptFailed event is emitted when initializing a connection fails
    • connectionAttemptTimeout event will not be emitted since the family autoselection feature is not implemented
    • The addressType argument in the connectAttempt and connectionAttmptFailed events is the correct type
    • Writes while connecting are buffered until connection established, then buffered writes are flushed
    • Writev works
    • Writes can be corked/uncorked any time
    • Writes can be encoded strings or TypedArrays, other types throw
    • Individual write callbacks are called. Error argument is set appropriately when write fails
    • Calling end() closes the connection
    • The drain event is emitted when the write buffer is empty
    • When end() is called, buffered data in the readable side is still avaialble
    • If options.lookup is provided and destination address is not an IP address, the options.lookup function should be called to "resolve" the address. (optional... we could just as easily choose not to support options.lookup)
    • The lookup event is emitted after the options.lookup is called and returns a result
    • Underlying reader on the socket is a BYOB reader reusing the user provided buffer or our auto allocated buffer
    • An error while reading from the underlying socket causes the net.Socket to be destroyed with an error
    • The net.Socket can have a read event attached putting the stream in flowing mode
    • If the read event is attached while the connection is still pending, the read will automatically start when the connection is established.
    • The stream can be paused/resumed to stop/restart the flow of data
    • Attempting to connect to a pipe path fails
    • The data event is emitted when a chunk of data has been read
    • The end event is emitted when the underlying stream readable side is done
    • Reading honors backpressure signaling by pausing
    • Receiving and EOS from the remote should end the readable side of the stream. If allowHalfOpen is false, this should close and destroy the socket ONLY once the write buffer is drained. If allowHalfOpen is true, it should not.
    • socket.setTimeout(...) sets an activity timer on the socket.
    • The socket timeout is reset on writes, reads, and when the connection is established.
    • The timeout event is emitted when the activity timeout expires.
    • socket.address() returns the local address if the socket is connected, undefined otherwise... we hard code this to 0.0.0.0:0 since we have no notion of a local bound address in workers
    • socket.autoSelectFamilyAttemptedAddresses is always an array, when connect is called, it always just contains the one address we're connecting to. This is generally non-op since we do not implement family autoselection
    • socket.bufferSize returns the number of bytes buffered
    • socket.bytesRead returns the number of bytes read
    • socket.bytesWritten returns the number of bytes written
    • socket.connect(options[, connectListener]) works
      • Passing an ipv6 address works
      • options.autoSelectFamily only accepts a falsy value. truthy values throw
      • options.autoSelectFamilyAttemptTimeout is validated to be a number but is otherwise ignored
      • options.family is verified to match the host specified
      • options.hints is ignored unless options.lookup is given, then it is passed to that function when called
      • options.host is the address to connect to. Defaults to localhost.
      • options.keepAlive only falsy values are accepted. Truthy values throw
      • options.keepAliveInitialDelay is validated to be a number but is otherwise ignored
      • options.localAddress is ignored
      • options.localPort is ignored
      • options.lookup is used if the host is not a valid IP address
      • options.noDelay falsy values are accepted. Truthy values throw
      • options.port is validated to be a proper port
      • options.path throws
      • options.onread is used if specified
    • socket.connect(path[, connectListener]) throws
    • socket.connect(port[, host[, connectListener]) works
    • socket.connecting is true while the connection is being established. Is false otherwise
    • socket.destroy(error) immediately destroys the stream and closes the connection
      • All underlying resources are freed
    • socket.connect(...) can be called again immediately after socket.destroy(...) Currently not planning to implement this for now. We can do this in a separate PR.
    • socket.destroyed is true after calling socket.destroy()
    • socket.destroySoon() destroys the socket after all data is written. If the finish event already emitted, the socket is destroyed immediately. If the socket is still writable, end() is called.
    • socket.end(data[, encoding[, callback]]) writes the given chun and ends the writable size and half-closes the socket
    • socket.localAddress always returns 0.0.0.0
    • socket.localPort always returns 0
    • socket.localFamily always returns 0
    • socket.pause() pauses reading of data on the stream. Pauses the underlying read loop
    • socket.pending is true if the socket is not connected yet at all
    • socket.ref() and socket.unref() are non-op
    • socket.remoteAddress is the remote host
    • socket.remoteFamily is the remote host IP type
    • socket.remotePort is the remote port
    • socket.resetAndDestroy() closes the connection and destroys the stream.
    • socket.resume() resumes reading after a call to pause
    • socket.setEncoding(...) sets the text encoding for the readable side of the socket
    • socket.setKeepAlive() accepts falsy values, throws on truthy values
    • socket.setNoDelay() accepts falsy values, throws on truthy values
    • socket.readyState accurately reflects the ready status of the connection
    • The timeout timer is reset after each read from the underlying socket
  • Add the compat flag / date that will signal the availability of the net API`
  • Tests test and more tests ... The plan is to port as many of the Node.js tests as possible but testing of the Socket API within workerd is fairly limited currently
  • Documentation

Future work items (to come in separate follow-up PRs

  • tls.TLSSocket is implemented
  • net.BlockList is implemented
  • net.SocketAddress is implemented
  • Socket supports diagnostics channel
  • AsyncLocalStorage context is correctly propagated such that it matches Node.js' behavior (will needs tests to verify this)

@jasnell
jasnell force-pushed the jsnell/node-net branch 7 times, most recently from 298ff8f to a2a10e5 Compare June 5, 2024 21:56
@jasnell jasnell changed the title [WIP] Initial scaffolding for built-in node:net impl [WIP] Implement built-in node:net impl Jun 5, 2024
@jasnell
jasnell force-pushed the jsnell/node-net branch 11 times, most recently from 220d9b3 to e035eb0 Compare June 11, 2024 16:41
@jasnell
jasnell marked this pull request as ready for review June 11, 2024 16:44
@jasnell
jasnell requested review from a team as code owners June 11, 2024 16:44
@jasnell
jasnell requested review from Frederik-Baetens, IgorMinar, dario-piotrowicz and mikea and removed request for Frederik-Baetens June 11, 2024 16:44
@jasnell

This comment was marked as resolved.

Implements the connect handler and tcp-ingress for a worker

See the samples/tcp-ingress for an example
@jasnell
jasnell force-pushed the jsnell/node-net branch 11 times, most recently from 155b5b7 to 5370184 Compare June 15, 2024 02:37
@jasnell

jasnell commented Jun 17, 2024

Copy link
Copy Markdown
Collaborator Author

This PR can be reviewed. Landing is blocked on the pending code review of #1429

@dom96 dom96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is tough to review, but I think great testing coverage is what is likeliest to find bugs anyway. You're planning to give this great test coverage so we should be good on that front.

I think the structure of the implementation is good, so approving.

"nodejs_compat_v2",
# The nodejs_compat_net flag explicitly enables the node:net module.
# The nodejs_compat or nodejs_compat_v2 flags must also be set.
"nodejs_compat_net",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sucks a bit that we need to have a separate compat flag for this. Is there any way we could avoid this and have this new net implementation be part of nodejs_compat_v2?

@jasnell jasnell Jun 20, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is part of the strategy to more seamlessly integrate with wrangler. The idea is to use the compat flags as a way of signaling to wrangler that a new node.js compat API is available. In this case it also helps us gate the availability based on use of the experimental flag.

cc @IgorMinar

Comment thread src/node/net.js
Comment thread src/node/net.js
Comment thread src/node/net.js
Comment thread src/node/net.js Outdated
Comment thread src/node/net.js Outdated
Comment thread src/node/net.js
@jasnell

jasnell commented Jun 20, 2024

Copy link
Copy Markdown
Collaborator Author

Note that this PR is still blocked pending review of #1429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants