Skip to content
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.

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

node::net.Socket() reusing socket works on NodeJS but fails in Bun #7325

Closed
wackfx opened this issue Nov 26, 2023 · 8 comments · Fixed by #10781
Closed

node::net.Socket() reusing socket works on NodeJS but fails in Bun #7325

wackfx opened this issue Nov 26, 2023 · 8 comments · Fixed by #10781
Labels
bug Something isn't working node.js Compatibility with Node.js APIs

Comments

@wackfx
Copy link

wackfx commented Nov 26, 2023

What version of Bun is running?

1.0.13

What platform is your computer?

Darwin 22.6.0 arm64 arm

What steps can reproduce the bug?

Reusing a client socket connection works in Node.JS - but fails in Bun.

Reusing means that we close the connection at some point for some reasons then we try to reconnect and reuse the socket.
This use case is often implemented in database clients to avoid stale socket issues.

Test script - you should have a simple echo TCP server running

// Run tcp server with `docker run --rm -d -p 9000:9000 venilnoronha/tcp-echo-server:latest`
import net from 'node:net'

// Pretty standard socket stuff & wait util
const socket = new net.Socket()
function on_data(data) {
    console.log('data', data.toString())
}
function bind_listeners() {
    socket.on('data', on_data)
}
function remove_listeners() {
    socket.removeListener('data', on_data)
}
async function wait(ms) {
    return new Promise((ok, ko) => {
        setTimeout(ok, ms)
    })
}

// Actual test code
// we want to simulate a standard 1) socket connection 2) writing data 3) eventually close
// In a real world scenario, the socket would end much later (in a setTimeout for ex)
// but for this exemple to have the error faster, we trigger the end right after we wrote some data
// NB: It still give an error even if you do a longer period before connect(ing) and end(ing)
async function send_hello() {
    return new Promise((ok, ko) => {
        // binding event - connecting's done at the end
        socket.once('connect', (...args) => {
            console.log('connected')
            // Binding data listener - NodeJS fails without it (might be a lead for some shenanigans?)
            bind_listeners()
            socket.write('script\n', (err) => {
                if (err) return ko(err)
                console.log('wrote')
                // remove added listeners & end connection
                remove_listeners()
                socket.end(ok)
            })
        })
        socket.connect(9000, '127.0.0.1')
    })
}

// Try to perform this action 3 times
async function main() {
    await send_hello()
    await wait(1000)
    await send_hello()
    await wait(1000)
    await send_hello()
}
await main()

What is the expected behavior?

No error (like in node)
image
image

What do you see instead?

It fails with Bun
image

Additional information

Found this while debugging a pretty famous node module that froze over a period of time in Bun but worked flawlessly on Node.JS.

A solution we found to bypass this error was to recreate a new.Socket() each time after a socket "ended" (instead of reusing the same socket)

@wackfx wackfx added the bug Something isn't working label Nov 26, 2023
@Electroid Electroid added the node.js Compatibility with Node.js APIs label Nov 27, 2023
@deanrih
Copy link

deanrih commented Feb 18, 2024

It seem this is still the issue on the latest version 1.0.27

❯ node --version; node ../test.js
v20.10.0
connected
wrote
connected
wrote
connected
wrote
❯ bun --version; bun run ../test.js
1.0.27
connected
wrote
connected
28 |         // binding event - connecting's done at the end
29 |         socket.once('connect', (...args) => {
30 |             console.log('connected')
31 |             // Binding data listener - NodeJS fails without it (might be a lead for some shenanigans?)
32 |             bind_listeners()
33 |             socket.write('script\n', (err) => {
                 ^
error: write after end
 code: "ERR_STREAM_WRITE_AFTER_END"

      at new NodeError (node:stream:405:39)
      at _write (node:stream:2546:39)
      at node:stream:2579:79
      at /mnt/tier_2_common/workspace/test/test.js:33:13
      at open (node:net:74:17)

@evelant
Copy link

evelant commented Apr 1, 2024

For anyone else who lands here with an issue using postgres-js on Bun, 3.4.4 will not release the connection when you call end. You can roll back to 3.4.3 which will release the connection but will fail after ~1hr or so if you keep it open.

@wiesys
Copy link

wiesys commented May 17, 2024

Hmm... the issue with Postgres.js not properly closing the connection for me is present even in Bun v1.1.8. Are you sure @Jarred-Sumner that it is fixed?

Looks like I'm not the only one: porsager/postgres#817 (comment)

@amjed-ali-k
Copy link

amjed-ali-k commented May 21, 2024

Postgres.js 3.4.3 | Bun 1.1.7 | 20 Mins
image

Postgres.js 3.4.4 | Bun 1.1.7 | 15 Mins
image

Postgres.js 3.4.4 | Bun 1.1.8 | 5 Mins
image
The error only happened to me when server is in idle. 0 Req/min.

Postgres.js 3.4.4 | Bun 1.1.9 (canary) | 24 Mins
image

Postgres.js 3.4.4 | Bun 1.1.12
image

Postgres.js 3.4.4 | Bun 1.1.20
image

@deanrih
Copy link

deanrih commented May 22, 2024

@amjed-ali-k

Hi, may I have the code you are using to test it and also perhaps the postgresql config if any (maybe such us timeout and such), I want to test it on my end (which I know probably behave the same) and also trying some workaround I'm currently trying as well to validate it

Also can confirm, for me it has higher chance of the issue being triggered when the server was idling

@amjed-ali-k
Copy link

amjed-ali-k commented May 23, 2024

@amjed-ali-k

Hi, may I have the code you are using to test it and also perhaps the postgresql config if any (maybe such us timeout and such), I want to test it on my end (which I know probably behave the same) and also trying some workaround I'm currently trying as well to validate it

Also can confirm, for me it has higher chance of the issue being triggered when the server was idling

@deanrih
Created a fork of one of your repo.
https://github.com/amjed-ali-k/bun-elysia-drizzle-postgres-test

Add your posgres db url in .env file.
run bun run db:generate & bun run db:push

Here is the screenshot of requests i made to the server. Check the time difference also. DB read request after 25 Mins got me this error.
image

Also, i didn't got any error in locally hosted prostres. (for 30 mins)
You can find my pg_config dump in db-config-dump.txt

@deanrih
Copy link

deanrih commented May 23, 2024

@amjed-ali-k
Hi, may I have the code you are using to test it and also perhaps the postgresql config if any (maybe such us timeout and such), I want to test it on my end (which I know probably behave the same) and also trying some workaround I'm currently trying as well to validate it
Also can confirm, for me it has higher chance of the issue being triggered when the server was idling

@deanrih Created a fork of one of your repo. https://github.com/amjed-ali-k/bun-elysia-drizzle-postgres-test

Add your posgres db url in .env file. run bun run db:generate & bun run db:push

Here is the screenshot of requests i made to the server. Check the time difference also. DB read request after 25 Mins got me this error. image

Also, i didn't got any error in locally hosted prostres. (for 30 mins) You can find my pg_config dump in db-config-dump.txt

@amjed-ali-k

Oh wow, sorry for the late reply, and thank you, didn't expect for my dummy test repo to be used and kinda forgot about it (playing around with the local copy), will try it soon

@amjed-ali-k
Copy link

This issue seems fixed in bun 1.1.39 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working node.js Compatibility with Node.js APIs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants