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

knex uses synchronous API #3595

Closed
tsondergaard opened this issue Dec 27, 2019 · 5 comments
Closed

knex uses synchronous API #3595

tsondergaard opened this issue Dec 27, 2019 · 5 comments

Comments

@tsondergaard
Copy link
Contributor

@tsondergaard tsondergaard commented Dec 27, 2019

Running node with --trace-sync-io for an application that uses knex outputs warnings such as this one:

(node:10232) WARNING: Detected use of sync API
    at randomBytes (internal/crypto/random.js:54:31)
    at nodeRNG (/home/ts/src/easyviz/easyviz-backend/node_modules/uuid/lib/rng.js:7:17)
    at v4 (/home/ts/src/easyviz/easyviz-backend/node_modules/uuid/v4.js:13:52)
    at toSQL (/home/ts/src/easyviz/easyviz-backend/node_modules/knex/lib/query/compiler.js:73:28)
    at toSQL (/home/ts/src/easyviz/easyviz-backend/node_modules/knex/lib/query/builder.js:77:44)
    at /home/ts/src/easyviz/easyviz-backend/node_modules/knex/lib/runner.js:30:36
    at /home/ts/src/easyviz/easyviz-backend/node_modules/knex/lib/runner.js:253:24
    at tryCatcher (/home/ts/src/easyviz/easyviz-backend/node_modules/knex/node_modules/bluebird/js/release/util.js:16:23)
    at /home/ts/src/easyviz/easyviz-backend/node_modules/knex/node_modules/bluebird/js/release/promise.js:547:31

Using a synchronous API is undesirable even if the randomBytes() function is normally very fast. If for no other reason, just for the fact that it makes --trace-sync-io much less useful when knex creates a lot of noise.

Looking at the use if uuid.v4() in knex master branch (commit a613fe2):

[ts@flash knex]$ git grep uuid.v4 -- lib
lib/query/compiler.js:      __knexQueryUid: uuid.v4(),
lib/raw.js:    obj.__knexQueryUid = uuid.v4();
[ts@flash knex]$ git grep __knexQueryUid -- lib
lib/query/compiler.js:      __knexQueryUid: uuid.v4(),
lib/raw.js:    obj.__knexQueryUid = uuid.v4();

Is __knexQueryUid actually used?

If it is, does it require just uniqueness from the uuid or the additional cryptographic randomness of uuid v4? If uniqueness is sufficient a uuid.v1() will provide that just as well as uuid.v4(), but 5 times faster and without triggering the --trace-sync-io warning.

Here is a small perf experiment comparing v1 and v4 uuid generation:

$ cat experiment.js 
const uuid = require("uuid");
const { performance } = require("perf_hooks");

const iterations = 5000000;

let t_begin = performance.now();
for (let i = 0; i < iterations; ++i) {
    uuid.v1();
}
console.log(`${iterations} uuid.v1(): ${(performance.now() - t_begin).toFixed(0)} ms`);

t_begin = performance.now();
for (let i = 0; i < iterations; ++i) {
    uuid.v4();
}
console.log(`${iterations} uuid.v4(): ${(performance.now() - t_begin).toFixed(0)} ms`);

$ node experiment.js 
5000000 uuid.v1(): 2941 ms
5000000 uuid.v4(): 14589 ms


@elhigu

This comment has been minimized.

Copy link
Member

@elhigu elhigu commented Dec 27, 2019

Is __knexQueryUid actually used?

It is used to allow to certain event handlers to know that some events are happening to the same query.

Perf is not an issue here, since it is not any bottleneck in knex query execution. But getting rid off the WARNING: Detected use of sync API warning is good enough reason to change it.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Dec 27, 2019

@tsondergaard Would you consider opening PR for this?

@tsondergaard

This comment has been minimized.

Copy link
Contributor Author

@tsondergaard tsondergaard commented Dec 27, 2019

Yes, I will be happy to.

tsondergaard added a commit to tsondergaard/knex that referenced this issue Dec 27, 2019
Running a program that uses knex with node --trace-sync-io results in
warnings such as this one:

    (node:10232) WARNING: Detected use of sync API
        at randomBytes (internal/crypto/random.js:54:31)
        at nodeRNG ([...]/node_modules/uuid/lib/rng.js:7:17)
        at v4 ([...]/node_modules/uuid/v4.js:13:52)
        at toSQL ([...]/node_modules/knex/lib/query/compiler.js:73:28)
        at toSQL ([...]/node_modules/knex/lib/query/builder.js:77:44)
        at [...]/node_modules/knex/lib/runner.js:30:36
        at [...]/node_modules/knex/lib/runner.js:253:24
        at tryCatcher ([...]/node_modules/knex/node_modules/bluebird/js/release/util.js:16:23)
        at [...]/node_modules/knex/node_modules/bluebird/js/release/promise.js:547:31

uuid.v1() provides more than adequate collision guarantees, is 5 times
faster than uuid.v4() and most importantly do not trigger the warning
above.
@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Dec 28, 2019

fixed in #3596

@kibertoad kibertoad closed this Dec 28, 2019
@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Dec 28, 2019

Released in 0.20.5

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