Skip to content

Conversation

@alexlamsl
Copy link
Contributor

  • support TCP sockets for now, i.e. no IPC
  • extra features like keep-alive, no-delay etc. are absent due to limitations of uSockets
  • fix jest to treat done(nullish) as success

@Jarred-Sumner
Copy link
Collaborator

extra features like keep-alive, no-delay etc. are absent due to limitations of uSockets

We can change this manually ourselves but it's fine for a follow-up. We would call setsocketopt on the file descriptor (which we would need to handle specially for TLS), which is how usockets itself would do it.

@Jarred-Sumner
Copy link
Collaborator

done(nullish)

Is that consistent with what Jest does?

@alexlamsl
Copy link
Contributor Author

Is that consistent with what Jest does?

AFAICT, yes.

@alexlamsl alexlamsl changed the title implement net.Socket [WIP] implement net.Socket Jan 2, 2023
@alexlamsl alexlamsl force-pushed the net-tcp-socket branch 2 times, most recently from 81b7104 to 658837d Compare January 2, 2023 22:18
@alexlamsl alexlamsl changed the title [WIP] implement net.Socket implement net.Socket Jan 2, 2023
const self = socket.data;
self.bytesRead += buffer.length;
if (self.#readQueue.isEmpty()) {
if (self.push(buffer)) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we return Buffer objects in the stream class? We could add a flag to do that here because IIRC, node:stream will internally create a Buffer object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically Uint8Array (so basically Buffer from Node.js PoV) − will investigate if node:stream does extra allocation or not 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah it does in the streams code when you push a Uint8Array, it creates a new Buffer

- support TCP sockets for now, i.e. no IPC
- extra features like keep-alive, no-delay etc. are absent due to limitations of uSockets
- fix `jest` to treat `done(nullish)` as success
}

_write(chunk, encoding, callback) {
if (typeof chunk == "string" && encoding !== "utf8") chunk = Buffer.from(chunk, encoding);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we split this into two functions it might be faster:

  1. if (typeof chunk === "string") this.#writeString(chunk, callback)
  2. this.#writeBuffer(chunk, callback)

We could also add a DOMJIT binding for writeText / writeBytes, as we have done for ServerWebSocket and then we can have lower function call overhead in these functions. It would only shave off around 20ns though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, the above code is there to handle things like socket.setEncoding("hex").write("deadbeef"), i.e. for correctness rather than performance

@Jarred-Sumner Jarred-Sumner merged commit 983b747 into oven-sh:main Jan 2, 2023
@alexlamsl alexlamsl deleted the net-tcp-socket branch January 2, 2023 22:55
Jarred-Sumner pushed a commit that referenced this pull request Jan 6, 2023
- support TCP sockets for now, i.e. no IPC
- extra features like keep-alive, no-delay etc. are absent due to limitations of uSockets
- fix `jest` to treat `done(nullish)` as success
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants