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 double readyForQuery #2420

Merged
merged 3 commits into from
Nov 30, 2020
Merged

Fix double readyForQuery #2420

merged 3 commits into from
Nov 30, 2020

Conversation

brianc
Copy link
Owner

@brianc brianc commented Nov 25, 2020

This is fixing a double readyForQuery message being sent from the backend (because we were calling sync after an error, which I already fixed in the main driver). I also pulled in the reproduction case for another error from #2333, and I'm going to work on fixing that one as well.

@brianc
Copy link
Owner Author

brianc commented Nov 25, 2020

Included a test case for #2333 to reproduce the issue and then turned the test green. The issue was a queued stream which the client had not submitted was having an error triggered on it when .end was called on the client. The client errors out all waiting queries if you .end the client while there are queued queries. The entire query queue concept is an unfortunate API decision I made like 9 years ago & it causes all sorts of confusing problems like this. I'd like to remove the ability to queue queries at some point in the future. It doesn't make a lot of sense to me as you're queuing up a bunch of stuff that may or may not work in the future depending on future states & makes the API a bunch more complicated. A lot of this was before there were things like async/await (or even before the async library was widely used) as a "convenience" thing which...yeah...bad idea. The problem w/ removing it is it's a subtle & large-ish breaking change and also a ton of my early unit and integration tests rely on the behavior. So, will be a big undertaking to remove it. Anyways - this fixes another weird edge cases w/ queued query canceling.

@brianc brianc marked this pull request as ready for review November 25, 2020 22:52
@brianc brianc merged commit 4fde8b7 into master Nov 30, 2020
@brianc brianc deleted the bmc/cursor-stream-error-handling branch November 30, 2020 15:25
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.

1 participant