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.

Sign up for GitHub

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

Jump to bottom

pool.query(new QueryStream(...)) unexpected behaviour #2013

Open
m-ronchi opened this issue Nov 27, 2019 · 6 comments · May be fixed by #2810
Open

pool.query(new QueryStream(...)) unexpected behaviour #2013

m-ronchi opened this issue Nov 27, 2019 · 6 comments · May be fixed by #2810
Labels
bug

Comments

@m-ronchi
Copy link

Hi,
if I use the query method on pg.Pool with a query object that implements sumbit (i.e. pg-query-stream or pg-cursor) like this:

import { Pool } from "pg";
import QueryStream = require("pg-query-stream");
const pool = new Pool();
// ...
const query = new QueryStream("SELECT * FROM ...");
const q = pool.query(query);
console.log(q); // Promise { <pending>, ...
await q; //!

the returned promise never resolves and the pool never releases the used client

I could ignore the return value and continue with the result, e.g.

for await (const row in query) { ...process row...}

but the client leak is a problem

furthermore, the typescript definition is plain wrong:

export class Pool extends events.EventEmitter {
// ...
    query<T extends Submittable>(queryStream: T): T;
// ...
}
@brianc brianc added the bug label Nov 27, 2019
@brianc
Copy link
Owner

brianc commented Nov 27, 2019

yeah that sounds like a problem - I'll definitely look at this. For now I think a workaround you can do is

const client = await pool.connect()
const query = new QueryStream('SELECT *')
const stream = client.query(query)
for await (const row of stream) {

}
client.release()

Sorry for the hassle - i'll get this fixed

@m-ronchi
Copy link
Author

that's what I did (wrapped in try-finally)

@timothyallan
Copy link

Just running into this now with pg@8.5.1. and pg-query-stream@3.4.2, is the workaround above still the best option?

@MelissaSnell
Copy link

MelissaSnell commented Mar 29, 2021 •
edited

I am wondering if this is the cause of the issue that I am seeing on pg@8.5.1. I have a codebase that appears to run OK but the issue appears in functional testing. Our unit tests actually run against a database and we use pool.query to run queries. We have a lot of unit tests and about 3/4 of the way through we see :

(node:4274) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 end listeners added to [Connection]. Use emitter.setMaxListeners() to increase limit at _addListener (events.js:281:17) at Connection.addListener (events.js:297:10) at Connection.once (events.js:328:8) at /Users/username/GitHub/my-code-base/node_modules/pg/lib/client.js:604:25 at new WrappedPromise (/Users/username/GitHub/my-code-base/node_modules/async-listener/es6-wrapped-promise.js:13:18) at Client.end (/Users/username/GitHub/my-code-base/node_modules/pg/lib/client.js:603:14) at BoundPool._remove (/Users/username/GitHub/my-code-base/node_modules/pg-pool/index.js:157:12) at Timeout.<anonymous> (/Users/username/GitHub/my-code-base/node_modules/pg-pool/index.js:324:14) at Timeout._onTimeout (/Users/username/GitHub/my-code-base/node_modules/async-listener/glue.js:188:31) at listOnTimeout (internal/timers.js:531:17) at processTimers (internal/timers.js:475:7)
Our pg.query is like this:

async query(text, values){ const res = await this.pool.query(text, values); return res; }

Is there anything that I can add to our function to ensure it is releasing things correctly (if this is in fact the issue)? Many thanks.

I have tried replacing pool.query with getting a client and then releasing it but this doesn't resolve the issue

aleclarson added a commit to aleclarson/node-postgres that referenced this issue Sep 7, 2022
@aleclarson
fix(pg-query-stream): invoke this.callback on cursor end/error
c036d07 
Closes brianc#2013
@aleclarson aleclarson linked a pull request Sep 7, 2022 that will close this issue
Open
@aleclarson
Copy link

I've submitted a PR that fixes this 👀

#2810

aleclarson added a commit to aleclarson/node-postgres that referenced this issue Dec 5, 2023
@aleclarson
fix(pg-query-stream): invoke this.callback on cursor end/error
e55da72 
Closes brianc#2013
@abenhamdine
Copy link
Contributor

Run into this today as well, it tooks me several hours to figure out why (and yet I know well this library !)

It would be great to finalize the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(pg-query-stream): invoke this.callback on cursor end/error aleclarson/node-postgres
6 participants
@brianc @MelissaSnell @aleclarson @timothyallan @abenhamdine @m-ronchi