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: flow control in message-io.js #878

Merged
merged 1 commit into from
Mar 16, 2019
Merged

fix: flow control in message-io.js #878

merged 1 commit into from
Mar 16, 2019

Conversation

chdh
Copy link
Collaborator

@chdh chdh commented Mar 14, 2019

Fix for #877

@arthurschreiber could you have a look at this? In Tedious 2.7.0 you changed the data flow architecture to a stream of streams. The inner stream must also be paused.

@chdh chdh requested a review from arthurschreiber March 14, 2019 22:46
@arthurschreiber
Copy link
Collaborator

Thank you @chdh for preparing this, and sorry for having broken this with the 2.7.0 release. Do you have a suggestion how we could add a test to ensure we don't regress on this again on the future?

@arthurschreiber
Copy link
Collaborator

@chdh FYI, I tweaked the implementation a tiny bit.

@arthurschreiber
Copy link
Collaborator

🎉 This PR is included in version 6.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@chdh
Copy link
Collaborator Author

chdh commented Mar 16, 2019

@Arthur Thanks for your fix. This is the better solution. And thanks for merging and releasing so quickly.

@chdh chdh deleted the chdh/flow-control-fix branch March 16, 2019 10:46
@chdh
Copy link
Collaborator Author

chdh commented Mar 16, 2019

Do you have a suggestion how we could add a test to ensure we don't regress on this again on the future?

We could do the same thing as in my test program (published with #877): Start a very large query, pause it and see if the memory increases signiffically. But that would be a relatively unstable and unreliable method.

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

Successfully merging this pull request may close these issues.

2 participants