Fix context between two async requests #7
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daniloisr
force-pushed
the
handle-async-context
branch
from
9a1c94e to
5e41987
Compare
added 3 commits
November 20, 2018 16:17
This commit fixes a bug where a request can end without finishing its timings. See: MiniProfiler#4 Bug cause: The structure that is used to register Miniprofiler timing providers, like (Postgres, HTTP, Redis), because it overrides the original method (globally) and uses the `request` object to access the Miniprofiler extensions to build the timings, and this doesn't work in a scenario with simultaneous requests using an async provider. Here is an example using [`pg`](https://github.com/goenning/miniprofiler-pg/blob/master/index.js) to try illustrating the failing scenario (check out the `tests/concurrent-async-test.js` test to see it running). request A start: * `pg.Client.prototype.query` holds a `req` object of requestA. * It calls `.query` in a pg instance * A miniprofiler timing starts with the call to `req.miniprofiler.timeQuery(...)` * The original method is called (async). request B start: * `pg.Client.prototype.query` holds a `req` object of request B. * It calls `.query` in a pg instance * Start timing with `req.miniprofiler.timeQuery(...)` * The original method is called (async). request A resume: * The result of `.query` is returned * A new call to `.query` is made * This time the `req` points to request B, this means that `req.miniprofiler.timeQuery(...)` will start a timing on request B. * The original method is called (async) request B resume: * The result of `.query` is returned. * All data was fetched, the request is ready to finish, so internally Miniprofile calls [`stopProfilling`](https://github.com/MiniProfiler/node/blob/1a98e40698b1126ac8de728a33406656361f8870/lib/miniprofiler.js#L80). * This fails because there is a timing started (by request A) but not finished, so calculating the [diffs](https://github.com/MiniProfiler/node/blob/1a98e40698b1126ac8de728a33406656361f8870/lib/miniprofiler.js#L409) will fails because `stop` is undefined. Solution -------- Using NodeJS "async_hooks" we can track the reference to the correct extension for each request, so calls to `req.miniprofiler.timeQuery()` will point to the correct miniprofiler extension. To check some performance analisys see: nodejs/node#14794 (comment) The goal of the current commit isn't introduce breaking changes, so the miniprofiler reference is injected into the request using JS getters. Another solution is changing the API for providers, where instead of receiving a `req` reference, they can receive a function that gets the reference to the correct miniprofiler instance. But this will break API with all existing providers. References ---------- - https://medium.com/the-node-js-collection/async-hooks-in-node-js-illustrated-b7ce1344111f - https://medium.com/@guysegev/async-hooks-a-whole-new-world-of-opportunities-a1a6daf1990a - nodejs/node#14794 (comment)
Koa adds dependency to `"debug": "*"` which fetch a version non compatible with Node v4. Locking it on our package.json fix this issue.
daniloisr
force-pushed
the
handle-async-context
branch
from
5e41987 to
44e8197
Compare
After more testing the 'async-hooks' polyfill shown to not work properly for Node version prior to 8.
daniloisr
pushed a commit
to goenning/miniprofiler-redis
that referenced
this pull request
Nov 27, 2018
With the addition of `async_hooks` [0] the `req.miniprofiler` is now a getter, and some Redis methods don't plays nice with it. [0]: MiniProfiler/node#7
|
@NickCraver thank you very much for merging this one 🙇♂️ |
|
@daniloisr sorry for the delay, I've add as you maintainer of miniprofiler package. AFAIK, the deploy process is manual |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes a bug where a request can end without finishing its timings. See: #4
Bug cause: The structure that is used to register Miniprofiler providers, like (Postgres, HTTP, Redis, ...), because it overrides the original method (globally) and uses the
requestobject to access theMiniprofiler extensions to build the timings, and this doesn't work in a scenario with simultaneous requests using an async provider.
Here is an example using
pgto try illustrating the failing scenario (checktests/concurrent-async-test.jstest to see it running).request A start (it's expected to run two DB queries)
pg.Client.prototype.queryholds areqobject of requestA..queryin a pg instancereq.miniprofiler.timeQuery(...)(let's call this timing A-1)request B start (it's expected to run a single DB query)
pg.Client.prototype.queryholds areqobject of request B..queryin a pg instancereq.miniprofiler.timeQuery(...)(timing B-1)request A resume
.queryis returned (timing A-1 is stoped).queryis madereqpoints to request B, this means thatreq.miniprofiler.timeQuery(...)starts a timing on request B. (timing B-2)request B resume
.queryis returned (timing B-1 is stoped)stopProfilling.stopis undefined.Solution
Using NodeJS "async_hooks" we can track the reference to the correct extension for each request, so calls to
req.miniprofiler.timeQuery()will point to the correct miniprofiler extension.To check some performance analysis see:
nodejs/node#14794 (comment)
The goal of the current commit isn't introduce breaking changes, so the miniprofiler reference is injected into the request using JS getters.
Another solution is changing the API for providers, where instead of receiving a
reqreference, they would receive a function to get the reference to the correct miniprofiler instance. But this will break API with all existing providers.References