fix for #2500 #2505

capaj · 2018-02-21T20:00:17Z

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 · 2018-02-21T20:05:19Z

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:

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.

wubzz · 2018-02-21T20:18:17Z

src/util/make-knex.js

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

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

capaj · 2018-03-20T01:22:24Z

@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 · 2018-03-20T07:06:47Z

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

wubzz · 2018-03-20T07:07:06Z

src/interface.js

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


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

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.

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.

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 · 2018-03-20T08:53:13Z

I'm fine with merging this given explanation above.

@elhigu Ok for you too?

capaj · 2018-03-20T09:59:11Z

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

capaj · 2018-03-20T15:44:40Z

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

elhigu · 2018-03-21T12:37:31Z

src/interface.js

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


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 · 2018-03-21T14:07:52Z

@elhigu added the throw as requested

elhigu

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

capaj · 2018-03-21T15:30:53Z

@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 · 2018-04-12T09:22:13Z

So is this PR actually going to be accepted?

elhigu · 2018-04-12T09:28:18Z

@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 · 2018-04-12T09:39:45Z

@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 · 2018-04-12T10:29:03Z

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 · 2018-04-12T14:48:03Z

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

jpike88 · 2018-04-13T04:42:31Z

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

elhigu · 2018-04-13T05:42:00Z

@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 · 2018-04-13T07:19:58Z

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

elhigu · 2018-04-13T07:30:58Z

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 · 2018-04-17T15:20:38Z

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

capaj · 2018-04-21T11:01:41Z

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

elhigu · 2018-04-25T21:03:37Z

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 · 2018-04-25T21:11:19Z

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

jpike88 · 2018-04-26T04:11:48Z

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

elhigu · 2018-04-26T16:04:06Z

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

jpike88 · 2018-05-22T05:26:14Z

@elhigu one month later...

jpike88 · 2018-05-28T04:55:23Z

@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 · 2018-05-28T06:13:03Z

@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 · 2018-05-28T06:17:38Z

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 · 2018-05-28T07:01:21Z

@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 · 2018-05-28T07:04:40Z

Awesome, thanks

chaintng · 2018-06-13T05:05:13Z

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 · 2018-08-23T17:41:38Z

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

knex#2505 in the knex repo

capaj force-pushed the async-stack-trace-for-errors branch from ac5ce0b to 72347ed Compare February 21, 2018 20:10

wubzz reviewed Feb 21, 2018

View reviewed changes

capaj force-pushed the async-stack-trace-for-errors branch from 72347ed to a28603a Compare February 22, 2018 11:46

capaj force-pushed the async-stack-trace-for-errors branch from a28603a to 05bebc0 Compare March 20, 2018 01:19

capaj changed the title ~~Hackfix for #2500~~ fix for #2500 Mar 20, 2018

wubzz reviewed Mar 20, 2018

View reviewed changes

capaj force-pushed the async-stack-trace-for-errors branch from 05bebc0 to 10d8d9d Compare March 20, 2018 08:49

capaj force-pushed the async-stack-trace-for-errors branch 2 times, most recently from c743aeb to 5c74cf4 Compare March 20, 2018 10:58

capaj mentioned this pull request Mar 20, 2018

save stack trace to the client code in QueryBuilder and replace a stack if knex is configured with asyncStackTraces Vincit/objection.js#845

Closed

elhigu requested changes Mar 21, 2018

View reviewed changes

capaj force-pushed the async-stack-trace-for-errors branch from 5c74cf4 to 2eb2669 Compare March 21, 2018 14:06

elhigu approved these changes Mar 21, 2018

View reviewed changes

elhigu requested changes Apr 12, 2018

View reviewed changes

fixes knex#2500

cb00544

capaj force-pushed the async-stack-trace-for-errors branch from 2eb2669 to cb00544 Compare April 17, 2018 15:19

elhigu approved these changes Apr 25, 2018

View reviewed changes

elhigu merged commit e086427 into knex:master Apr 25, 2018

rclmenezes mentioned this pull request Dec 18, 2019

Async errors don't show full stack trace Vincit/objection.js#1618

Closed

JakobJoonas pushed a commit to JakobJoonas/knex that referenced this pull request Sep 23, 2022

Added documentation for asyncStackTraces config (knex#104)

bfa8ef1

knex#2505 in the knex repo

fix for #2500 #2505

fix for #2500 #2505

Conversation

capaj commented Feb 21, 2018 • edited

capaj commented Feb 21, 2018 • edited

wubzz Feb 21, 2018

Choose a reason for hiding this comment

capaj Feb 22, 2018

Choose a reason for hiding this comment

capaj commented Mar 20, 2018

wubzz commented Mar 20, 2018

wubzz Mar 20, 2018

Choose a reason for hiding this comment

capaj Mar 20, 2018

Choose a reason for hiding this comment

capaj Mar 20, 2018

Choose a reason for hiding this comment

elhigu Mar 21, 2018

Choose a reason for hiding this comment

wubzz commented Mar 20, 2018

capaj commented Mar 20, 2018

capaj commented Mar 20, 2018

elhigu left a comment

Choose a reason for hiding this comment

elhigu Mar 21, 2018

Choose a reason for hiding this comment

capaj commented Mar 21, 2018

elhigu left a comment

Choose a reason for hiding this comment

capaj commented Mar 21, 2018 • edited

jpike88 commented Apr 12, 2018

elhigu commented Apr 12, 2018

elhigu left a comment

Choose a reason for hiding this comment

capaj commented Apr 12, 2018

jpike88 commented Apr 12, 2018

capaj commented Apr 12, 2018

jpike88 commented Apr 13, 2018

elhigu commented Apr 13, 2018

capaj commented Apr 13, 2018

elhigu commented Apr 13, 2018

capaj commented Apr 17, 2018

capaj commented Apr 21, 2018

elhigu commented Apr 25, 2018

elhigu commented Apr 25, 2018

jpike88 commented Apr 26, 2018

elhigu commented Apr 26, 2018

jpike88 commented May 22, 2018

jpike88 commented May 28, 2018

elhigu commented May 28, 2018

jpike88 commented May 28, 2018

capaj commented May 28, 2018

jpike88 commented May 28, 2018

chaintng commented Jun 13, 2018

capaj commented Aug 23, 2018

capaj commented Feb 21, 2018 •

edited

capaj commented Feb 21, 2018 •

edited

capaj commented Mar 21, 2018 •

edited