Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aleclarson
Copy link

@aleclarson aleclarson commented Sep 7, 2022

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

@brianc
Copy link
Owner

brianc commented Sep 8, 2022

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
Copy link

peterp commented Sep 11, 2022

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

@aleclarson
Copy link
Author

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

@peterp
Copy link

peterp commented Sep 19, 2022

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

@brianc
Copy link
Owner

brianc commented Sep 27, 2022

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
Copy link

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

@aleclarson
Copy link
Author

@ashmit-coder 👍

@brianc
Copy link
Owner

brianc commented Dec 8, 2023

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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