search

snyk / nodejs-lockfile-parser

Generate a Snyk dependency tree from package-lock.json or yarn.lock file
Other
60 stars 28 forks source link

feat: intro of eventloopSpinner lib. #61

Closed anthogez closed 4 years ago

anthogez commented 4 years ago

What this does

replacement of setImmediate with event-loop-spinner to solve current event-loop blocking issues

michael-go commented 4 years ago

This probably won't help with the event-loop block, maybe only if the this.treeSize % this.eventLoopSpinRate === 0 was happening too rarely.

looking at the blocking stacktrace, there are no long loops we might be missing where the spinner can be added too? (I can't look right now myself, will do later a deeper look)

anthogez commented 4 years ago

This probably won't help with the event-loop block, maybe only if the this.treeSize % this.eventLoopSpinRate === 0 was happening too rarely.

looking at the blocking stacktrace, there are no long loops we might be missing where the spinner can be added too? (I can't look right now myself, will do later a deeper look)

I was considering these points:

acyclicDuplicationRec https://github.com/snyk/nodejs-lockfile-parser/blob/fix/event-loop-issue/lib/parsers/package-lock-parser.ts#L200

createGraphOfDependencies https://github.com/snyk/nodejs-lockfile-parser/blob/fix/event-loop-issue/lib/parsers/package-lock-parser.ts#L278

michael-go commented 4 years ago

The most frequent stacktrace recorded during blocks by NSolid is:

     at createGraphOfDependencies (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:216:13)
    at /srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:54:35
    at /srv/app/node_modules/tslib/tslib.js:113:75
    at __awaiter (/srv/app/node_modules/tslib/tslib.js:109:16)
    at getDependencyTree (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:28:24)
    at /srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/index.js:50:31
    at /srv/app/node_modules/tslib/tslib.js:113:75
    at __awaiter (/srv/app/node_modules/tslib/tslib.js:109:16)
    at buildDepTree (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/index.js:20:20
   ...

so worth adding a spinner inside this inner loop: https://github.com/snyk/nodejs-lockfile-parser/blob/4b848954f58521cc9d4f073b60c656cb8df22ffe/lib/parsers/package-lock-parser.ts#L329

unfortunately, most other blocks I saw are probably happenning inside 3rd party libs, where we can't just put a spinner in, and a more complex solution is needed, like this stack trace:

 at Graph.predecessors (/srv/app/node_modules/graphlib/lib/graph.js:255:40)
    at visit (/srv/app/node_modules/graphlib/lib/alg/topsort.js:19:16)
    at arrayEach (/srv/app/node_modules/lodash/_arrayEach.js:15:9)
    at forEach (/srv/app/node_modules/lodash/forEach.js:38:10)
    at visit (/srv/app/node_modules/graphlib/lib/alg/topsort.js:19:9)
    at arrayEach (/srv/app/node_modules/lodash/_arrayEach.js:15:9)
    at forEach (/srv/app/node_modules/lodash/forEach.js:38:10)
    at visit (/srv/app/node_modules/graphlib/lib/alg/topsort.js:19:9)
    at arrayEach (/srv/app/node_modules/lodash/_arrayEach.js:15:9)
    at forEach (/srv/app/node_modules/lodash/forEach.js:38:10)
    at visit (/srv/app/node_modules/graphlib/lib/alg/topsort.js:19:9)
    at arrayEach (/srv/app/node_modules/lodash/_arrayEach.js:15:9)
    at forEach (/srv/app/node_modules/lodash/forEach.js:38:10)
    at topsort (/srv/app/node_modules/graphlib/lib/alg/topsort.js:25:5)
    at /srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:253:43
    at /srv/app/node_modules/tslib/tslib.js:113:75
    at __awaiter (/srv/app/node_modules/tslib/tslib.js:109:16)
    at createDepTrees (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:243:24)
    at /srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:67:60
    at /srv/app/node_modules/tslib/tslib.js:113:75
    at __awaiter (/srv/app/node_modules/tslib/tslib.js:109:16)
    at getDependencyTree (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:28:24)
    at /srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/index.js:50:31
    at /srv/app/node_modules/tslib/tslib.js:113:75
    at __awaiter (/srv/app/node_modules/tslib/tslib.js:109:16)
    at buildDepTree (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/index.js:20:20)
   ...
anthogez commented 4 years ago

The most frequent stacktrace recorded during blocks by NSolid is:

     at createGraphOfDependencies (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:216:13)
    at /srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:54:35
    at /srv/app/node_modules/tslib/tslib.js:113:75
    at __awaiter (/srv/app/node_modules/tslib/tslib.js:109:16)
    at getDependencyTree (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:28:24)
    at /srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/index.js:50:31
    at /srv/app/node_modules/tslib/tslib.js:113:75
    at __awaiter (/srv/app/node_modules/tslib/tslib.js:109:16)
    at buildDepTree (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/index.js:20:20
   ...

so worth adding a spinner inside this inner loop:

https://github.com/snyk/nodejs-lockfile-parser/blob/4b848954f58521cc9d4f073b60c656cb8df22ffe/lib/parsers/package-lock-parser.ts#L329

unfortunately, most other blocks I saw are probably happenning inside 3rd party libs, where we can't just put a spinner in, and a more complex solution is needed, like this stack trace:

 at Graph.predecessors (/srv/app/node_modules/graphlib/lib/graph.js:255:40)
    at visit (/srv/app/node_modules/graphlib/lib/alg/topsort.js:19:16)
    at arrayEach (/srv/app/node_modules/lodash/_arrayEach.js:15:9)
    at forEach (/srv/app/node_modules/lodash/forEach.js:38:10)
    at visit (/srv/app/node_modules/graphlib/lib/alg/topsort.js:19:9)
    at arrayEach (/srv/app/node_modules/lodash/_arrayEach.js:15:9)
    at forEach (/srv/app/node_modules/lodash/forEach.js:38:10)
    at visit (/srv/app/node_modules/graphlib/lib/alg/topsort.js:19:9)
    at arrayEach (/srv/app/node_modules/lodash/_arrayEach.js:15:9)
    at forEach (/srv/app/node_modules/lodash/forEach.js:38:10)
    at visit (/srv/app/node_modules/graphlib/lib/alg/topsort.js:19:9)
    at arrayEach (/srv/app/node_modules/lodash/_arrayEach.js:15:9)
    at forEach (/srv/app/node_modules/lodash/forEach.js:38:10)
    at topsort (/srv/app/node_modules/graphlib/lib/alg/topsort.js:25:5)
    at /srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:253:43
    at /srv/app/node_modules/tslib/tslib.js:113:75
    at __awaiter (/srv/app/node_modules/tslib/tslib.js:109:16)
    at createDepTrees (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:243:24)
    at /srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:67:60
    at /srv/app/node_modules/tslib/tslib.js:113:75
    at __awaiter (/srv/app/node_modules/tslib/tslib.js:109:16)
    at getDependencyTree (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/parsers/package-lock-parser.js:28:24)
    at /srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/index.js:50:31
    at /srv/app/node_modules/tslib/tslib.js:113:75
    at __awaiter (/srv/app/node_modules/tslib/tslib.js:109:16)
    at buildDepTree (/srv/app/node_modules/snyk-nodejs-lockfile-parser/dist/index.js:20:20)
   ...

this will be intro in a new pr

snyksec commented 4 years ago

:tada: This PR is included in version 1.21.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: