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

Calling .end on a client (or pool) does not wait for connections to close #3287

Open
JamesMcNee opened this issue Jul 31, 2024 · 3 comments
Open

Comments

@JamesMcNee
Copy link

Hi there, it seems that there is a bug around client.end / pool.end in that when awaited (or using callbacks) the promise can resolve before the underlying connection / stream has disconnected.

Digging through the code a bit, it seems like the issue is in this bit of code:

end() {
// 0x58 = 'X'
this._ending = true
if (!this._connecting || !this.stream.writable) {
this.stream.end()
return
}
return this.stream.write(endBuffer, () => {
this.stream.end()
})
}

I think here we should hook into the callback from the sockets end event: https://nodejs.org/api/net.html#socketenddata-encoding-callback and ensure that the original promise does not resolve (or callback fire) before observing this.

@opyate
Copy link

opyate commented Aug 5, 2024

Yes, please add a callback to client.release() so I can pass resolve and wrap it in a Promise (or better yet, make release return a Promise).

The issue manifested itself like this for me: I use this package to verify DB writes in E2E tests, and Jest had this error:

Jest did not exit one second after the test run has completed.

Running Jest with --detectOpenHandles then showed:

Jest has detected the following 1 open handle potentially keeping Jest from exiting:

  ●  TCPWRAP

       7 |
       8 | async function withTransaction(it: Function) {
    >  9 |   const client = await pool.connect()
         |                             ^
      10 |   try {
      11 |     await client.query('BEGIN')
      12 |     await it(client)

      at Connection.connect (node_modules/pg/lib/connection.js:44:17)
      at Client._connect (node_modules/pg/lib/client.js:113:11)
      at Client.connect (node_modules/pg/lib/client.js:162:12)
      at BoundPool.newClient (node_modules/pg-pool/index.js:237:12)
      at BoundPool.connect (node_modules/pg-pool/index.js:212:10)
      at withTransaction (src/helpers/db.ts:9:29)
My code for reference
async function withTransaction(it: Function) {
  const client = await pool.connect()
  try {
    await client.query('BEGIN')
    await it(client)
    await client.query('COMMIT')
  } catch (error) {
    console.error('Error in transaction:', error)
    await client.query('ROLLBACK')
    throw error
  } finally {
    client.release()
  }
}

@JamesMcNee
Copy link
Author

JamesMcNee commented Aug 5, 2024

@opyate the Pool suffers from the same issue I raised, however your issue may be more around you not calling .end on the pool. When you .release a client, it simply returns it to the pool so that it might be reused, therefore, depending on your pool configuration, it is valid for the connection to remain open.

@opyate
Copy link

opyate commented Aug 6, 2024

Ah, of course, I missed .end in the doc, thanks 👍

Here's my solution for anyone else landing here.

File setupFilesAfterEnv.ts:

import { Pool } from 'pg'

beforeAll(() => {
  (global as any).pool = new Pool();  
});

afterAll(async () => {
  await (global as any).pool.end();
});

And my helper becomes:

const client = await (global as any).pool.connect()

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

No branches or pull requests

2 participants