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

Projects
None yet
2 participants
@novemberborn
Copy link
Contributor

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.

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

This comment has been minimized.

@elhigu

elhigu Aug 23, 2017

Collaborator

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.

@elhigu
Copy link
Collaborator

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!

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

@novemberborn novemberborn force-pushed the novemberborn:stream-emit-error branch from d6273ce to d907c72 Aug 24, 2017

@novemberborn

This comment has been minimized.

Copy link
Contributor Author

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?

@elhigu

elhigu approved these changes Aug 24, 2017

@elhigu

This comment has been minimized.

Copy link
Collaborator

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?

@novemberborn

This comment has been minimized.

Copy link
Contributor Author

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

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

This comment has been minimized.

@elhigu

elhigu Aug 27, 2017

Collaborator

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?

@elhigu
Copy link
Collaborator

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

@novemberborn

This comment has been minimized.

Copy link
Contributor Author

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.

@novemberborn

This comment has been minimized.

Copy link
Contributor Author

novemberborn commented Sep 28, 2017

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

});
});

promise.then(restore, restore);

This comment has been minimized.

@elhigu

elhigu Sep 28, 2017

Collaborator

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

This comment has been minimized.

@novemberborn

novemberborn Oct 2, 2017

Author Contributor

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

This comment has been minimized.

@elhigu

elhigu Oct 2, 2017

Collaborator

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 ?

This comment has been minimized.

@novemberborn

novemberborn Oct 3, 2017

Author Contributor

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.

@elhigu

elhigu approved these changes Oct 3, 2017

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Oct 3, 2017

All good now, thank you!

@elhigu elhigu merged commit 0ccd591 into tgriesser:master Oct 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@novemberborn novemberborn deleted the novemberborn:stream-emit-error branch Oct 4, 2017

elhigu added a commit to ivanslf/knex that referenced this pull request Oct 16, 2017

Fix connection error propagation when streaming (tgriesser#2199)
* 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

@elhigu elhigu referenced this pull request Oct 30, 2017

Closed

propagate pg stream errors #1345

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.