Skip to content

fix(pg-query-stream): invoke this.callback on cursor end/error#2810

Merged
brianc merged 6 commits into
brianc:masterfrom
aleclarson:fix/2013
Feb 13, 2026
Merged

fix(pg-query-stream): invoke this.callback on cursor end/error#2810
brianc merged 6 commits into
brianc:masterfrom
aleclarson:fix/2013

Conversation

@aleclarson

@aleclarson aleclarson commented Sep 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #2013

This issue was caused by two things:

  • the Client wasn't using the callback argument that Pool#query passes with the QueryStream submittable
  • the QueryStream wasn't attaching end/error listeners to its cursor, which it needs for calling its callback

@aleclarson aleclarson mentioned this pull request Sep 7, 2022
Closed
@brianc

brianc commented Sep 8, 2022

Copy link
Copy Markdown
Owner

Thanks for doing this! Can you include a test to reproduce the issue / prove its fixed? Without tests I am unlikely to merge PRs as they become a future bug waiting to re-occur! 😄

@peterp

peterp commented Sep 11, 2022

Copy link
Copy Markdown

I think pg-copy-streams may suffer from the same issue, and am happy to provide a failure case/ test.

@aleclarson

aleclarson commented Sep 11, 2022

Copy link
Copy Markdown
Contributor Author

@peterp I believe you can send a PR to my fork and it'll show up here when I merge

@peterp

peterp commented Sep 19, 2022

Copy link
Copy Markdown

@aleclarson I'm sorry; I'm not going to find enough time to write this.

@brianc

brianc commented Sep 27, 2022

Copy link
Copy Markdown
Owner

I'm sorry; I'm not going to find enough time to write this.

While I greatly appreciate the work in fixing this, the PR will remain unmerged until a test case is submitted. I can't merge untested code as it becomes a forever maintenance burden for me in the future with no way to check for expected behavior.

@ashmit-coder

ashmit-coder commented Dec 4, 2023

Copy link
Copy Markdown

@aleclarson Can you update your repo to latest. I maybe able to write the tests that we needed for this

@aleclarson

aleclarson commented Dec 5, 2023

Copy link
Copy Markdown
Contributor Author

@ashmit-coder 👍

@brianc

brianc commented Dec 8, 2023

Copy link
Copy Markdown
Owner

@aleclarson I let CI run on this PR so it should letcha know if tests pass. 😄

@brianc

brianc commented Feb 13, 2026

Copy link
Copy Markdown
Owner

BETTER LATE THAN NEVER! 🤦

@brianc
brianc merged commit 01e0556 into brianc:master Feb 13, 2026
15 checks passed
joshkel added a commit to joshkel/knex that referenced this pull request Feb 28, 2026
@joshkel
Fix pg-query-stream 4.13.0 incompatibility
f16c53e 
pg-query-stream 4.13.0 updates its query stream handling to invoke its
callback under more circumstances. This causes awaiting on a Knex
Postgres stream to return early.

See brianc/node-postgres#2810

This is reproducible with the current Knex test suite, if run against
pg-query-stream 4.13.0 instead of an older version.
@joshkel joshkel mentioned this pull request Feb 28, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@aleclarson @brianc @peterp @ashmit-coder