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 for #2500 #2505

Merged
merged 1 commit into from Apr 25, 2018

Conversation

Projects
None yet
5 participants
@capaj
Contributor

capaj commented Feb 21, 2018

slightly nasty way of getting better error stacks when using async/await.
Toggles on by setting a new config parameter called: asyncStackTraces to true

@capaj

This comment has been minimized.

Contributor

capaj commented Feb 21, 2018

I have only tested it with the most minimal example. I suspect we're going to need to iterate on this couple of times. Currently when I run it with the sample file I presented in the #2500 , I get this stacktrace:

(node:10109) UnhandledPromiseRejectionWarning: error: relation "userss" does not exist
    at test (/home/capaj/git_projects/tests/knex.js:17:9)
    at Object.<anonymous> (/home/capaj/git_projects/tests/knex.js:20:1)
    at Module._compile (module.js:660:30)
    at Object.Module._extensions..js (module.js:671:10)
    at Module.load (module.js:573:32)
    at tryModuleLoad (module.js:513:12)
    at Function.Module._load (module.js:505:3)
    at Function.Module.runMain (module.js:701:10)
    at startup (bootstrap_node.js:193:16)
    at bootstrap_node.js:617:3
(node:10109) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)

Which takes me straight to the beginning of my query in the editor:
image

I am sure you can spot lot of potential problems-I opened this mostly just to present what I have so far. It will certainly need some serious testing to be done. Just let me know if I am going in the right/wrong direction.

@@ -14,6 +14,14 @@ export default function makeKnex(client) {
// The object we're potentially using to kick off an initial chain.
function knex(tableName, options) {
const qb = knex.queryBuilder()
if (qb.client.config.asyncStackTraces) {

This comment has been minimized.

@wubzz

wubzz Feb 21, 2018

Collaborator

This unfortunately won't work for SchemaBuilder or Raw since this function is strictly QueryBuilder.

This should result in that _asyncStack does not exist in the .then handler change here for SchemaBuilder / Raw.

This comment has been minimized.

@capaj

capaj Feb 22, 2018

Contributor

right, I'll add the _asyncStack generatin for SchemaBuilder and for Raw too. Thanks!

@capaj capaj changed the title from Hackfix for #2500 to fix for #2500 Mar 20, 2018

@capaj

This comment has been minimized.

Contributor

capaj commented Mar 20, 2018

@wubzz I've added it to all three constructors-so the async stack trace gets saved for raw and for SchemaBuilder too. Wrote 3 simple unit tests for it. If you could take look and review it before 0.14.5 that would be great.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 20, 2018

@capaj Looks fine to me. I would have preferred an integration test for when Node >= 8, but I guess at the same time this is just a hack to bypass what boils down to a Node/V8 issue, so maybe doesn't matter too much.

if (this.client.config.asyncStackTraces) {
result.catch((err) => {
err.originalStack = err.stack

This comment has been minimized.

@wubzz

wubzz Mar 20, 2018

Collaborator

Might be wrong, but shouldn't it also throw here?

This comment has been minimized.

@capaj

capaj Mar 20, 2018

Contributor

I did try both and it seems we don't want to throw.
Without throw, when I run this code:

const test = async () => {
  try {
    await db('jijoi').select()
  } catch (err) {
    console.log('caught')
  }
  console.log('can be caught')
}

test()

I get this:

caught
can be caught

but when I add the throw there:

caught
can be caught
Unhandled rejection error: relation "jijoi" does not exist
    at test (/home/capaj/git_projects/tests/knex.js:18:11)
    at Object.<anonymous> (/home/capaj/git_projects/tests/knex.js:25:1)
    at Module._compile (module.js:649:30)
    at Object.Module._extensions..js (module.js:660:10)
    at Module.load (module.js:561:32)
    at tryModuleLoad (module.js:501:12)
    at Function.Module._load (module.js:493:3)
    at Function.Module.runMain (module.js:690:10)
    at startup (bootstrap_node.js:194:16)
    at bootstrap_node.js:666:3

There's that unhandled rejection.

When I don't throw, it behaves well even when I don't try-catch:

const test = async () => {
  await db('jijoi').select()
  console.log('does not run')
}

test()

outputs:

Unhandled rejection error: relation "jijoi" does not exist
    at test (/home/capaj/git_projects/tests/knex.js:17:9)
    at Object.<anonymous> (/home/capaj/git_projects/tests/knex.js:21:1)
    at Module._compile (module.js:649:30)
    at Object.Module._extensions..js (module.js:660:10)
    at Module.load (module.js:561:32)
    at tryModuleLoad (module.js:501:12)
    at Function.Module._load (module.js:493:3)
    at Function.Module.runMain (module.js:690:10)
    at startup (bootstrap_node.js:194:16)
    at bootstrap_node.js:666:3

(node:31738) UnhandledPromiseRejectionWarning: error: relation "jijoi" does not exist
    at test (/home/capaj/git_projects/tests/knex.js:17:9)
    at Object.<anonymous> (/home/capaj/git_projects/tests/knex.js:21:1)
    at Module._compile (module.js:649:30)
    at Object.Module._extensions..js (module.js:660:10)
    at Module.load (module.js:561:32)
    at tryModuleLoad (module.js:501:12)
    at Function.Module._load (module.js:493:3)
    at Function.Module.runMain (module.js:690:10)
    at startup (bootstrap_node.js:194:16)
    at bootstrap_node.js:666:3
(node:31738) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:31738) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

This comment has been minimized.

@capaj

capaj Mar 20, 2018

Contributor

I think this has got to do with the fact that knex does not return a real promise-it just has methods then and catch so it's masquerading like a promise, but it's really not.

This comment has been minimized.

@elhigu

elhigu Mar 21, 2018

Collaborator

I think @wubzz was right that it should throw. Now this extra catch is just added as an additional branch how result is resolved.

So instead of adding this new .catch(..) handler as a separate branch which is then left dangling without having reference to the returned promise, it should be added like this:

result = result.catch(...)
@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 20, 2018

I'm fine with merging this given explanation above.

@elhigu Ok for you too?

@capaj

This comment has been minimized.

Contributor

capaj commented Mar 20, 2018

It's failing in the tests-I will fix it in the afternoon

@capaj

This comment has been minimized.

Contributor

capaj commented Mar 20, 2018

feel free to install it via @capaj/knex@next. I tested by running on our medium sized project(33 objection.js models) with 200 integration tests, passed with flying colors. Also knex errors now have useful stacktraces!

@elhigu

Sorry for taking long with this. I started work on new project that has nothing to do with javascript so I have lot less time to put in knex issues (I still have at least 10h/month though :) ).

if (this.client.config.asyncStackTraces) {
result.catch((err) => {
err.originalStack = err.stack

This comment has been minimized.

@elhigu

elhigu Mar 21, 2018

Collaborator

I think @wubzz was right that it should throw. Now this extra catch is just added as an additional branch how result is resolved.

So instead of adding this new .catch(..) handler as a separate branch which is then left dangling without having reference to the returned promise, it should be added like this:

result = result.catch(...)
@capaj

This comment has been minimized.

Contributor

capaj commented Mar 21, 2018

@elhigu added the throw as requested

@elhigu

elhigu approved these changes Mar 21, 2018

Looks good. Integration test would have been good to add, but I don’t see it that necessary in this PR.

@capaj

This comment has been minimized.

Contributor

capaj commented Mar 21, 2018

@elhigu I will try to make them run on my machine and add some when I have some spare time in the coming weeks. I've only added unit tests because those were easy to run without setting up any databases.

@jpike88

This comment has been minimized.

jpike88 commented Apr 12, 2018

So is this PR actually going to be accepted?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 12, 2018

@jpike88 well last event was about capaj was going to try to add integration test, and after that the new configuration options must be documented too. After that this is good to go.

@elhigu

needs related documentation PR

@capaj

This comment has been minimized.

Contributor

capaj commented Apr 12, 2018

@elhigu I'll add that documentation PR this evening. I can try adding integration test, but I currently get this error:

1) mariadb | mariasql "before all" hook:
     Error: Access denied for user 'root'@'localhost' (using password: NO)
      at Client_MariaSQL.acquireRawConnection (lib/dialects/maria/index.js:9:3584)
      at create (lib/client.js:9:16774)
      at tryPromise (node_modules/tarn/lib/Pool.js:347:22)
      at tryPromise (node_modules/tarn/lib/utils.js:57:20)
      at Promise (node_modules/tarn/lib/Pool.js:347:5)
      at new Promise (<anonymous>)
      at callbackOrPromise (node_modules/tarn/lib/Pool.js:338:10)
      at Pool._create (node_modules/tarn/lib/Pool.js:294:5)
      at Pool._doCreate (node_modules/tarn/lib/Pool.js:262:32)
      at Pool._tryAcquireOrCreate (node_modules/tarn/lib/Pool.js:202:12)
      at Pool.acquire (node_modules/tarn/lib/Pool.js:123:10)
      at lib/client.js:9:19730
  From previous event:
      at Client_MariaSQL.acquireConnection (lib/client.js:9:19620)
      at Runner.ensureConnection (lib/runner.js:9:11168)
      at Runner.run (lib/runner.js:9:2636)
      at SchemaBuilder.Target.then (lib/interface.js:9:1908)
      at Migrator.forceFreeMigrationsLock (lib/migrate/index.js:9:9685)
      at Context.<anonymous> (test/integration/migrate/index.js:15:25)

is there any documentation on how to set up an environment for knex integration tests @elhigu ?

@jpike88

This comment has been minimized.

jpike88 commented Apr 12, 2018

Im trying to include this in my project and having major issues... I'm using the esm module and its saying require(...) is not a function.

Isn't it as simple as just running npm install && npm build in the folder first?

@capaj

This comment has been minimized.

Contributor

capaj commented Apr 12, 2018

here's the doc PR: knex/documentation#104

@jpike88

This comment has been minimized.

jpike88 commented Apr 13, 2018

Requesting that a new release is made just to get this out there, it's going to be so damn useful

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 13, 2018

@capaj for mysql/mariadb you just need to setup mysql server and create knex_test database and has access there with user root without password. Database called knex_test is pretty much the only thing that every DB needs for being able to ran the tests against them. tests/knexfile.js has all the connection details how each DB is connected during the tests.

But if you want to run tests just for some databases you can use DB="postgres sqlite" npm run test in contributing.md is some stuff about how to setup postgres tables.

I suppose that would be good if I add some docker-compose file for setting up all databases for running tests.

@capaj

This comment has been minimized.

Contributor

capaj commented Apr 13, 2018

@elhigu thanks-I'll try to setup those docker images.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 13, 2018

https://github.com/tgriesser/knex/blob/master/scripts/stress-test/docker-compose.yml

there is already docker-compose file, but ports / usernames etc. should be changed and knex_test databases created for running normal tests against them.

@capaj

This comment has been minimized.

Contributor

capaj commented Apr 17, 2018

@elhigu added those integration tests here: cb00544#diff-bfeb3d958a56cecbae4f5f8983fdf195R713

@capaj

This comment has been minimized.

Contributor

capaj commented Apr 21, 2018

@elhigu do you still miss anything or can you merge this as is?

@elhigu

elhigu approved these changes Apr 25, 2018

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 25, 2018

That might be unstable, but lets see if it blows in future and thinks better way to do it then. I'll give this final review now :)

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 25, 2018

Tests probably should have still checked that this work for transactions too. But we'll find it out soon enough.

@elhigu elhigu merged commit e086427 into tgriesser:master Apr 25, 2018

1 check passed

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

This comment has been minimized.

jpike88 commented Apr 26, 2018

Awesome. @elhigu don’t want to sound pushy, do you have an rough eta of the next release?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 26, 2018

@jpike88 when I have time at some point, pretty soon

@jpike88

This comment has been minimized.

jpike88 commented May 22, 2018

@elhigu one month later...

@jpike88

This comment has been minimized.

jpike88 commented May 28, 2018

@elhigu bump...

are you the only person who is authorised to handle releases? if so, any idea on an eta? no pressure, I'd just fork and build it myself but there doesn't seem to be clear instructions on a build process.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 28, 2018

@jpike88 yes there are various persons who has rights. Bumping this repetitively actually sounds a bit like pressuring. I'm going to do some other stuff first before doing the next release that will be 0.15.0. Eta is definitely before september or maybe next week. This is not that critical issue that I would rush releasing just for this one.

@jpike88

This comment has been minimized.

jpike88 commented May 28, 2018

Is there an easy build guide or something in case I want to be more cutting edge? Would be nice if knex had a beta track or something

@capaj

This comment has been minimized.

Contributor

capaj commented May 28, 2018

@jpike88 it is published namespaced on @capaj/knex so you can use that until the next release. Build should be just about running npm run build if I recall correctly.

@jpike88

This comment has been minimized.

jpike88 commented May 28, 2018

Awesome, thanks

@chaintng

This comment has been minimized.

chaintng commented Jun 13, 2018

can someone please tell me where to put the config file?

i tried below but it still not working

var knex = require('knex')({
  dialect: 'mysql2',
  asyncStackTraces: true,
  connection: config.get('database.connectionString'),
});
@capaj

This comment has been minimized.

Contributor

capaj commented Aug 23, 2018

@chaintng that should work. It's a bit late now, but let me know if you still have issues-I'll gladly look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment