node:net rework #18962
node:net rework #18962
Conversation
|
Updated 4:04 PM PT - May 28th, 2025
❌ @nektro, your commit 6a07f7e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 18962That installs a local version of the PR into your bun-18962 --bun |
| @@ -1,5 +1,6 @@ | |||
| 'use strict'; | |||
| const common = require('../common'); | |||
| if (typeof globalThis.Bun !== "undefined") return; // TODO: BUN | |||
There was a problem hiding this comment.
| if (typeof globalThis.Bun !== "undefined") return; // TODO: BUN |
There was a problem hiding this comment.
we report openssl v1 but hit the error in the comment for openssl v3
| pub const ref = RefCount.ref; | ||
| pub const deref = RefCount.deref; | ||
| const ENABLE_AUTO_CORK = true; // ENABLE CORK OPTIMIZATION | ||
| const ENABLE_AUTO_CORK = false; // ENABLE CORK OPTIMIZATION |
There was a problem hiding this comment.
| const ENABLE_AUTO_CORK = false; // ENABLE CORK OPTIMIZATION | |
| const ENABLE_AUTO_CORK = true; // ENABLE CORK OPTIMIZATION |
We need corking otherwise performance will drop a lot
There was a problem hiding this comment.
mentioned above, i'll be implementing the AutoFlusher protocol and reevaluating corking post-merge
| pub fn close(this: *This, globalObject: *JSC.JSGlobalObject, callframe: *JSC.CallFrame) bun.JSError!JSValue { | ||
| JSC.markBinding(@src()); | ||
| _ = callframe; | ||
| this.socket.close(.normal); |
There was a problem hiding this comment.
should close wait until all buffered data is flushed? or is meant to immediately destroy the connection?
There was a problem hiding this comment.
lemme double check node's source
There was a problem hiding this comment.
calls https://docs.libuv.org/en/v1.x/handle.html#c.uv_close which destroys immediately.
perhaps i should make the js call shutdown and remove this method
There was a problem hiding this comment.
oh thats different from https://docs.libuv.org/en/v1.x/stream.html#c.uv_shutdown
and another spot in the js calls https://docs.libuv.org/en/v1.x/tcp.html#c.uv_tcp_close_reset
There was a problem hiding this comment.
do you have a preferred path forward?
adds 40 new passing node tests, more to be added in followup
Fixes #1796
Fixes #16079
Fixes #16564
Fixes #2809
Fixes #16943
Fixes #4660
Fixes #19545