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

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

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. 😄

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

Successfully merging this pull request may close these issues.

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