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

Merged
merged 9 commits into from Jun 27, 2018

Conversation

Projects
None yet
4 participants
@fcmatteo
Contributor

fcmatteo commented May 31, 2018

It should fix #2109 and a case when there is an error running the query.

})
it('emits error if not passed a function and the query has wrong bindings', function(done) {
setTimeout(() => {

This comment has been minimized.

@kibertoad

kibertoad May 31, 2018

Collaborator

Doesn't mocha timeout itself? You can use this.timeout() to adjust it for a specific test.

This comment has been minimized.

@fcmatteo

fcmatteo May 31, 2018

Contributor

Right, fixing that.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented May 31, 2018

LGTM. @elhigu What do you think?

connection.query(queryOptions, obj.bindings).stream(options).pipe(stream)
const queryStream = connection.query(queryOptions, obj.bindings).stream(options)
queryStream.on('error', (err) => {

This comment has been minimized.

@kibertoad

kibertoad May 31, 2018

Collaborator

Is it only relevant for MySQL?

This comment has been minimized.

@fcmatteo

fcmatteo May 31, 2018

Contributor

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.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented May 31, 2018

@fcmatteo Tests are failing on Mariadb, I presume same fix should be applied to it or exclusion added (since I think there were plans to remove maria dialect altogether).

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented May 31, 2018

@fcmatteo Almost there, two failing test on postgres remain :)

@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented May 31, 2018

@kibertoad It's weird. In my machine, all the tests using postgres (10.4) passes.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 31, 2018

@fcmatteo have you ran npm run build; before running the tests? You can try with postgres 9.6 too.

if (isArray(sql)) {
if (hasHandler) throw err;
stream.emit('error', err);
try {

This comment has been minimized.

@elhigu

elhigu May 31, 2018

Collaborator

This change needs to be checked carefully...

This comment has been minimized.

@kibertoad

kibertoad Jun 4, 2018

Collaborator

Which part looks dangerous to you? The fact that thrown exception is now being caught?

This comment has been minimized.

@kibertoad

kibertoad Jun 4, 2018

Collaborator

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

This comment has been minimized.

@elhigu

elhigu Jun 26, 2018

Collaborator

it was just a note for my futureself who has time to check this out better.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 31, 2018

@kibertoad I really don't know if this is correct or not without testing the branch myself and checking the errors from tests before the patch etc. Mostly changes seemed at least harmless, but the runner part change might cause some unexpected behaviour I need to think that part of code when I'm not tired.

@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented May 31, 2018

@elhigu Yes, I built before running the tests. Now I've tried with PostgreSQL 9.6.9 and passed too. 😕

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented May 31, 2018

@fcmatteo How are you running tests? Using npm?

@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented May 31, 2018

@kibertoad After building with npm run babel I run DB=postgres npm run test.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented May 31, 2018

@fcmatteo Checking out myself now, let's see if it passes for me as well.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented May 31, 2018

@fcmatteo It does pass locally - I wonder if issue is to CI server being slower than developer workstation or different order of test execution. Looking at the stracktrace, what seems to be happening is that in this block

  con.on('parseComplete', function (msg) {
    if (self.activeQuery.name) {

self.activeQuery is null. One of the possible causes for this is for

  const connectedErrorHandler = (err) => {
    if (this.activeQuery) {
      var activeQuery = self.activeQuery
      this.activeQuery = null
      return activeQuery.handleError(err, con)
    }
    this.emit('error', err)
  }

to get triggered. Hence my suspicion is that connection that has errored out in one of the tests doesn't get this processed before another test begins, effectively resetting activeQuery in the middle of the execution.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented May 31, 2018

btw, probably #2601 could also be covered? I wonder why it doesn't cause tests to explode for Oracle.

@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented Jun 1, 2018

I'm trying some things to debug but as I can't reproduce I need to commit. Sorry!

@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented Jun 1, 2018

@kibertoad OK it seems to be what you say. I destroyed the connection manually and the tests passed. Now we have to look for a real solution to the problem.
BTW, it seems to be a problem that existed before this PR but as there were no test cases where the query fails using streams, it was unnoticed.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Jun 1, 2018

@fcmatteo Now linting fails :)

home/travis/build/tgriesser/knex/src/dialects/postgres/index.js
  202:11  error  'client' is assigned a value but never used  no-unused-vars
@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented Jun 1, 2018

Forgot to delete that line. That is what I used to close the connection manually on error (called client.destroyRawConnection). Now I'm trying some things.

@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented Jun 1, 2018

It seems that I fixed it but now it's failing with Oracle in Node 6 and 8. I'll check that later.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Jun 1, 2018

@fcmatteo Yeah, that's to be expected. I presume that #2601 finally kicked in :)

@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented Jun 4, 2018

I'm going on vacation tonight so if there is something more I should change let me know as soon as possible.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Jun 4, 2018

@fcmatteo Woohoo, it passes! If you gave my write permissions on your fork, I could address any later comments in your absence.
Update: accepted, thanks!

}
return runner.client.stream(runner.connection, sql, stream, options)
} catch (e) {

This comment has been minimized.

@kibertoad

kibertoad Jun 4, 2018

Collaborator

@fcmatteo Is it intended that we are catching the error that we itself throw?

This comment has been minimized.

@fcmatteo

fcmatteo Jun 4, 2018

Contributor

It's the only way I found to emit the error to the stream. We could throw the error again in the catch, after emitting to the stream...

This comment has been minimized.

@kibertoad

kibertoad Jun 4, 2018

Collaborator

We probably should rethrow it to preserve the original logic, unless there is a good reason not to interrupt execution here.

@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented Jun 4, 2018

There's a problem with this test should remove the record in the lock table once finished. Any idea?

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Jun 4, 2018

@fcmatteo Is it another case of tests leaking? Does the test pass on its own?

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Jun 4, 2018

@fcmatteo Given that only node 8 is failing, I wonder if it's just a flaky test. Can you try rerunning tests by closing and reopening PR?

@fcmatteo fcmatteo closed this Jun 4, 2018

@fcmatteo fcmatteo reopened this Jun 4, 2018

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Jun 4, 2018

@fcmatteo Yup, looks like it was a flaky test after all.

@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented Jun 14, 2018

I'm back from vacations. Any chance we could merge the PR?

return runner.client.stream(runner.connection, sql, stream, options)
} catch (e) {
stream.emit('error', e)
throw new Error(e);

This comment has been minimized.

@elhigu

elhigu Jun 26, 2018

Collaborator

Why the error is wrapped to new Error before throwing?

This comment has been minimized.

@elhigu

elhigu Jun 26, 2018

Collaborator

probably re-throwing e is more compatible backwards + its easier for client to know which kind of db error actually was thrown

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 26, 2018

@fcmatteo I finally got time to check this out... other parts looks fine, but re-thrown exception should not be wrapped to new Error.

@fcmatteo

This comment has been minimized.

Contributor

fcmatteo commented Jun 26, 2018

@elhigu OK I removed the instantiation of the new Error and just throw the error.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 27, 2018

Thank you!

@elhigu elhigu merged commit ec85502 into tgriesser:master Jun 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
// '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();

This comment has been minimized.

@caseywebdev

caseywebdev Jul 3, 2018

Collaborator

Why was any of this changed? This query still needs to be closed on error or it will continue to execute on the server. See #1935

This comment has been minimized.

@elhigu

elhigu Jul 3, 2018

Collaborator

Good catch! @fcmatteo Any idea why queryStream.close() was dropped?

This comment has been minimized.

@kibertoad

kibertoad Jul 3, 2018

Collaborator

Ouch. Can we get a test for this?..

This comment has been minimized.

@elhigu

elhigu Jul 3, 2018

Collaborator

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.

This comment has been minimized.

@elhigu

elhigu Jul 3, 2018

Collaborator

@kibertoad you were faster 👍

This comment has been minimized.

@elhigu

elhigu Jul 3, 2018

Collaborator

Maybe by mocking queryStream.close method or by checking that stream.on('error'... event handler is set.

This comment has been minimized.

@elhigu

elhigu Jul 3, 2018

Collaborator

Or with integration test which queries for open connections from database.

This comment has been minimized.

@fcmatteo

fcmatteo Jul 3, 2018

Contributor

I don't remember. Following the thread of the PR, there were two failing tests in PostgreSQL that apparently were fixed by that.
But testing on local with the previous chunk of code seems to work too.

This comment has been minimized.

@elhigu

elhigu Jul 3, 2018

Collaborator

Ok, better to just put it back then and add new test

This comment has been minimized.

@kibertoad

kibertoad Aug 19, 2018

Collaborator

Followed up with #2773

veikovx added a commit to veikovx/knex that referenced this pull request Sep 17, 2018

Make the stream catch errors in the query (tgriesser#2638)
* Make the stream catch errors in the query

* Fix another case in which stream doesnt emits error

* Linter stuff

* Remove setTimeout in tests

* Make a test not to check the MySQL error code

* Fix stream error catching for MariaDB and PostgreSQL

* Fix stream error catching in Oracle

* Throw the error after emitting it to the stream

* Throw the error without instantiating a new Error

elhigu added a commit that referenced this pull request Sep 26, 2018

Kill queries after timeout for PostgreSQL (#2636)
* Kill queries after timeout for PostgreSQL

* Fix cancellation connection acquiring and test.

* Fix releasing connection in case query cancellation fails

* Add support for native enums on Postgres (#2632)

Reference https://www.postgresql.org/docs/current/static/sql-createtype.html

Closes #394

Signed-off-by: Will Soto <will.soto9@gmail.com>

* Removal of 'skim' (#2520)

* Allow overwriting log functions (#2625)

* Example build of custom log functions
* Handle logger object better for transactions
* Adjust test to ignore sqlite warning message

* Fixed onIn with empty values array (#2513)

* Drop support for strong-oracle (#2487)

* Remove babel-plugin-lodash (#2634)

While in theory, this may reduce the bundle size,
in practice it adds a ton of overhead during startup
due to the number of additional requires. Bundle
size also shouldn't matter for server side modules.

* Add json support to the query builder for mysql (#2635)

* Add json support to the query builder for mysql

refs #1036

Based on #1902

* Clarify supported version

* fix wrapIdentifier not being called in postgres alter column (#2612)

* fix wrapIdentifier not being called in postgres alter column

* add test for wrapIdentifier call in postgres alter column

* add comment regarding issue

* add issue & PR #'s in comment

* Remove readable-stream and safe-buffer (#2640)

* Add json support to the query builder for mysql

refs #1036

Based on #1902

* Clarify supported version

* Use native Buffer and Stream implementations

* fixes 2630 (#2642)

* Timeout errors shouldn't silently ignore the passed errors, but rather reject with original error. Fixes #2582 (#2626)

* chore: add Node.js 10 (#2594)

* chore: add Node.js 10

* chore: trigger new build

* chore: update lockfile

* chore: trigger new build

* fix: use npm i instead of npm ci

* Fix mssql driver crashing (#2637)

* Run SQL Server tests locally running SQL server in docker

* WIP mssql test stuff

* Patched MSSQL driver to not crash knex anymore

* Removed semicolon from rollback stmt for oracle (#2564)

* Remove WebSQL dialect (#2647)

* Add json support to the query builder for mysql

refs #1036

Based on #1902

* Clarify supported version

* Use native Buffer and Stream implementations

* Remove WebSQL dialect

* add homepage field to package.json (#2650)

* Make the stream catch errors in the query (#2638)

* Make the stream catch errors in the query

* Fix another case in which stream doesnt emits error

* Linter stuff

* Remove setTimeout in tests

* Make a test not to check the MySQL error code

* Fix stream error catching for MariaDB and PostgreSQL

* Fix stream error catching in Oracle

* Throw the error after emitting it to the stream

* Throw the error without instantiating a new Error

* Various fixes to mssql dialect (#2653)

* Fixed float type of mssql to be float

* Many tests where postgres test was not actually ran at all

* Migrations to be mssql compatible

Mssql driver doesn't handle if multiple queries are sent to same transaction concurrently.

* Prevented mssql failing when invalid schema builder was executed by accident

Instead of trying to generate sql from broken schema calls, just make exception to leak before query compiling is started

* Fixed mssql trx rollback to always throw an error

Also modified some connection test query to be mssql compatible

* Fixed various bugs from MSSQL driver to make tests run

* Fixed mssql unique index to be compatible with other dialect implementations

* Enable running mssql tests on CI

* Test for #2588

* Updated tests to not be dependend on tables left from previous test rans

* Trying to make mssql server work on travis

* Updated changelog and version

* Drop mariadb support  (#2681)

* Dropped support for mariasql

* ESLint

* Fixed docs to build again

* Fix knex.initialize() and adds test (#2477)

* Fix knex.initialize() and adds test

* Fix knex.initialize() and adds test

* Added test for reinitializing pool after destroy

* Fixed destroy / reinitialization test

* Fixed the fix

* Implement missing schema support for mssql dialect

* chore: Update dependencies. Remove estraverse (#2691)

* Update dependencies. Remove estraverse

* Fix compatibility with new Sinon

* Increase mssql timeout

* Normalized and validated driverNames of test db clients and fixed oracle test setup (#2692)

* Normalized and validated driverNames of test db clients and fixed oracle test setup

* Fixed failed queries from old query building tests which hadn't been ran in ages

* Allow selecting node version which is used to run oracledb docker tests

* Improved sql tester error messages

* Fixed rest of the oracledb tests

* Removed invalid flag from docker-compose

* Print mssql logs if initialization fails

* Fixed syntax error + final tests

* Added restart of failure for mssql DB initialization to try again if server was not ready

* Printout always mssql logs after container is started

* Fixed wait time printing after trying to connect

* Use npm run oracledb:test for testing oracle in travis

* Add Prettier (#2697)

* Add prettier
* Run files through prettier

* Istanbul -> NYC (#2700)

* istanbul -> nyc

* Update mocha

* Enforce code coverage (#2702)

* Enforce code coverage
* Update coverage numbers with current values

* version assignment on base class, copy on tx client, fix #2705

* Update changelog

* v0.15.1

* Added build step to test script. Fixes #2541 (#2712)

* Revert "Added build step to test script. Fixes #2541 (#2712)" (#2714)

This reverts commit 90ed8db.

* Allow oracle failures for now

* Fix issue with select(0) (#2711)

* Fix issue with select(0). Fixes #2658

* Refactor migrator (#2695)

* Refactor migrator

* Fix exports

* Fix ESLint

* Fix migrator

* Fix reference to config

* Split some more

* Fix table builder

* Fix argument order

* Merge branch 'master' of https://github.com/tgriesser/knex into feature/2690-support-multiple-migration-dirs

# Conflicts:
#	src/migrate/index.js
#	test/index.js
#	test/unit/migrate/migrator.js

* Fix #2715 (#2721)

* Fix #2715, explicitly set precision in datetime & timestamp
* Allow for precision in knex.fn.now, mysql time
* Add test for datetime with precision

* Bump changelog

* 0.15.2

* Fix issues with warnPromise when migration does not return a promise. Fixes #2725 (#2730)

* Add tests for multiple union arguments with callbacks and builders (#2749)

* Add tests for multiple union arguments

* Add some callback and raw tests to union unit tests

* Compile with before update so that bindings are put in correct order (#2733)

* Fix join using builder withSchema. (#2744)

* Use Datetime2 for MSSQL datetime + timestamp types (#2757)

* Use Datetime2 for MSSQL datetime + timestamp types

Datetime2 is now the recommended column type for new date work: https://docs.microsoft.com/en-us/sql/t-sql/data-types/datetime-transact-sql?view=sql-server-2017

* Add tests for MSSQL datetime2 changes

* General/document breaking change (#2774)

* Add changelog entry for a breaking change

* Improve entry

* Allow timestamp with timezone on mssql databases (#2724)

* Allow timestamp with timezone on mssql databases

* Change timestamp parameter to use object instead of boolean

* Update dependencies (#2772)

* Feature/2690: Multiple migration directories (#2735)

* Implement support for multiple migration directories

* Add tests for new functionality

* Fix tape tests

* Pass migration directory upwards

* Fix multiple directory support

* Fix bugs

* Rollback correctly

* Fix remaining tests

* Address comments

* #2758: Implement fail-fast logic for dialect resolution (#2776)

* Implement fail-fast logic for dialect resolution, clean-up code around.

* Remove method that was deprecated long time ago

* Address additional comments

* Try addressing comments

* Set client explicitly

* Fix compatibility with older Node versions

* Use columnize instead of wrap in using(). (#2713)

* Use columnize instead of wrap in using().

This is an attempt to fix #2136. Also added an integration test, couldn't find any existing.

* Exclude MSSQL from test as it does not support .using

* Change test to not use subquery

* Change test

* Introduced abstraction for getting migrations (#2775)

* Introduced abstraction for getting migrations

This would allow a webpack migration source which is compatible with bundling.

* Fixed migration validation with custom migration source

* Fixed issues after rebasing on muti directory PR

* Renamed parameter and fixed error message

* Addressed some PR comments

* Finished comment

* Moved filename extension filtering into fs-migrator

* Added test showing in memory custom migration source

* Cleaned up how to get config

* Fixed failing test

* Hopefully fix tests

* Fix Node.js 10 support in tests

* Test for correctly releasing cancel query connection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment