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 connection error propagation when streaming #2199

Merged
merged 3 commits into from Oct 3, 2017

Conversation

@novemberborn
Copy link
Contributor

@novemberborn novemberborn commented Aug 23, 2017

If stream() is used without a handler, make sure to emit connection errors on the stream itself.

This might fix #948, but I've only verified the patch when using Postgres.

src/runner.js Outdated
@@ -89,6 +92,17 @@ assign(Runner.prototype, {
stream.emit('error', err);
}
return runner.client.stream(runner.connection, sql, stream, options);
}).catch(function(err) {
Copy link
Member

@elhigu elhigu Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this catch would be in the same position where there was .catch(noop) earlier, then the if (hasHandler) throw err; line could be removed.

Loading

Copy link
Member

@elhigu elhigu left a comment

Thanks for the fix! Tests seems to be failing because of some linting error: 1:27 error 'noop' is defined but never used no-unused-vars

package-lock.json probably shouldn't be updated by thiss PR.

Also test case is necessary for this to be able to verify that fix actually does what it should do and that it won't get broken again.

Other that that, this looks good!

Loading

If stream() is used without a handler, make sure to emit connection
errors on the stream itself.
@novemberborn
Copy link
Contributor Author

@novemberborn novemberborn commented Aug 24, 2017

package-lock.json probably shouldn't be updated by thiss PR.

Oops! I usually git add -p but must have gotten lazy here 😉

Tests seems to be failing because of some linting error: 1:27 error 'noop' is defined but never used no-unused-vars

Removed.


I've cleaned up the catch and force-pushed to get rid of the package-lock.json. I'm not sure where to add a test for this though. I've found the stream integration tests but how would I trigger the connection error?

Loading

elhigu
elhigu approved these changes Aug 24, 2017
@elhigu
Copy link
Member

@elhigu elhigu commented Aug 24, 2017

Maybe connection error could be done by initializing knex instance with bad connection settings? That might work since we need ensureConnection() to throw an error... or maybe monkeypatching ensureConnection() for a moment could work too?

Loading

@novemberborn
Copy link
Contributor Author

@novemberborn novemberborn commented Aug 25, 2017

I must admit the test setup is a bit beyond me. I've pushed a commit which does a terrible monkey-patch. CI passes so… yay? 😄

Loading

return Promise.reject(expected);
};
var stream = knex('accounts').stream();
stream.on('error', function(actual) {
Copy link
Member

@elhigu elhigu Aug 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this could be wrapped to

return new Promise((resolve, reject) => { ... });

which will reject after 5ms or so unless error was emitted? That way test won't hang until mocha timeout and cleaning up could also restore the Runner.prototype.ensureConnection = original; even when test fails?

Loading

Copy link
Member

@elhigu elhigu left a comment

I'm glad that monkey patching helped testing this 👍 Still I would like to see one little change in the way how test is written (there is comment in the code about the details).

Loading

@novemberborn
Copy link
Contributor Author

@novemberborn novemberborn commented Sep 3, 2017

Just wanted to say I haven't forgotten about this. I ran into this in a client project, which I'm busy shipping at the moment. Will try and fix up the PR when that's done.

Loading

@novemberborn
Copy link
Contributor Author

@novemberborn novemberborn commented Sep 28, 2017

I've pushed a commit, hopefully it passes CI!

Loading

});
});

promise.then(restore, restore);
Copy link
Member

@elhigu elhigu Sep 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this catch even the rejected promise, so even if promise is rejected test will pass?

Loading

Copy link
Contributor Author

@novemberborn novemberborn Oct 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since the original promise is returned on the next line. This is a more manual Promise.prototype.finally.

Loading

Copy link
Member

@elhigu elhigu Oct 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so test just returns slightly before function is restored. I think it is fine though. Very unlikely to cause any problems. Is there reason not to use .finally ?

Loading

Copy link
Contributor Author

@novemberborn novemberborn Oct 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sequencing is such that the restore happens before the test finishes, because Mocha will attach the callbacks to the returned promise after the restore callbacks.

.finally isn't supported yet in any Node.js version.

Loading

elhigu
elhigu approved these changes Oct 3, 2017
@elhigu
Copy link
Member

@elhigu elhigu commented Oct 3, 2017

All good now, thank you!

Loading

@elhigu elhigu merged commit 0ccd591 into knex:master Oct 3, 2017
1 check passed
Loading
@novemberborn novemberborn deleted the stream-emit-error branch Oct 4, 2017
elhigu added a commit to ivanfilhoz/knex that referenced this issue Oct 16, 2017
* Fix connection error propagation when streaming

If stream() is used without a handler, make sure to emit connection
errors on the stream itself.

* Test stream error emission

* Improve test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants