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)