-
Notifications
You must be signed in to change notification settings - Fork 98
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
load cls before all else #187
Conversation
This ensures that the code async APIs get patched befored anything use-space can get loaded and interfere with tracing. This is a problem with `request` in particular as it caches setImmediate. Fixes: googleapis#182
@matthewloring I can't think of a useful test for this. A better idea would be an assertion at startup that iterates the module cache to find un-expected modules. Thoughts? PTAL regardless. |
Is it possible to distill a test case where the caching of setImmediate before cls can patch causes context loss? Looking through the module cache would help warn in some cases but I don't think it catches the issue fixed by this PR since we can't tell whether setImmediate has been stored from the module cache alone. It would be nice to include a test case mirroring the setImmediate issue that was discovered prompting this PR but if that is too unwieldy it looks good as is. |
It is not going to be unwieldy. It is going to be just un-useful in finding any other types of bugs. The issues was completely in the internals of our module and how npm 3 works. The test may guard against regressions of this exact bug, but it won't really find anything else. An assertion in our module itself would guard against that same regression, but more generally. |
What would the assertion look like? My understanding was that the issue here wasn't tied to a specific module being loaded that we could look for by name and it seems aggressive to crash if any module has been loaded before the agent. The assertion route sounds good as long as it can be made precise enough. |
I want to play with both approaches and see what ends up working well. But, since this bug is a release blocker, I have opened an issue (#189) to address the testing, thereby un-gating the release. |
LGTM |
Landing in #190 |
This ensures that the code async APIs get patched befored anything use-space can
get loaded and interfere with tracing. This is a problem with
request
inparticular as it caches setImmediate.
Fixes: #182