-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Make the stream catch errors in the query #2638
Changes from 7 commits
00d56ce
6b54639
2ffcd8f
02cd100
6562597
de8d019
04055b5
30fc639
06b41d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -199,15 +199,15 @@ assign(Client_PG.prototype, { | |
_stream(connection, obj, stream, options) { | ||
const PGQueryStream = process.browser ? undefined : require('pg-query-stream'); | ||
const sql = obj.sql; | ||
|
||
return new Promise(function(resolver, rejecter) { | ||
const queryStream = connection.query(new PGQueryStream(sql, obj.bindings, options)); | ||
queryStream.on('error', function(error) { stream.emit('error', error); }); | ||
// 'error' is not propagated by .pipe, but it breaks the pipe | ||
stream.on('error', function(error) { | ||
// Ensure the queryStream is closed so the connection can be released. | ||
queryStream.close(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was any of this changed? This query still needs to be closed on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! @fcmatteo Any idea why queryStream.close() was dropped? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ouch. Can we get a test for this?.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test case for original fix would have catch this that is the reason I'm so insistent that all the fixes has test included. So when this is fixed again, a test would be great to have. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kibertoad you were faster 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe by mocking queryStream.close method or by checking that stream.on('error'... event handler is set. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or with integration test which queries for open connections from database. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't remember. Following the thread of the PR, there were two failing tests in PostgreSQL that apparently were fixed by that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, better to just put it back then and add new test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Followed up with #2773 |
||
|
||
queryStream.on('error', function(error) { | ||
rejecter(error); | ||
stream.emit('error', error); | ||
}); | ||
|
||
// 'end' IS propagated by .pipe, by default | ||
stream.on('end', resolver); | ||
queryStream.pipe(stream); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,13 +84,17 @@ assign(Runner.prototype, { | |
const promise = Promise.using(this.ensureConnection(), function(connection) { | ||
hasConnection = true; | ||
runner.connection = connection; | ||
const sql = runner.builder.toSQL() | ||
const err = new Error('The stream may only be used with a single query statement.'); | ||
if (isArray(sql)) { | ||
if (hasHandler) throw err; | ||
stream.emit('error', err); | ||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change needs to be checked carefully... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which part looks dangerous to you? The fact that thrown exception is now being caught? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elhigu The only difference I see now (after latest change) is that it will now emit an error even if hasHandler is truthy. Would you consider that an unwelcome change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it was just a note for my futureself who has time to check this out better. |
||
const sql = runner.builder.toSQL() | ||
|
||
if (isArray(sql) && hasHandler) { | ||
throw new Error('The stream may only be used with a single query statement.'); | ||
} | ||
|
||
return runner.client.stream(runner.connection, sql, stream, options) | ||
} catch (e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fcmatteo Is it intended that we are catching the error that we itself throw? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the only way I found to emit the error to the stream. We could throw the error again in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably should rethrow it to preserve the original logic, unless there is a good reason not to interrupt execution here. |
||
stream.emit('error', e) | ||
} | ||
return runner.client.stream(runner.connection, sql, stream, options); | ||
}) | ||
|
||
// If a function is passed to handle the stream, send the stream | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it only relevant for MySQL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, I could reproduce the bug only in MySQL because that's the database I am using. The other bug, which I fixed editing
runner.js
, probably happens in other platforms too.