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

load cls before all else #187

Closed
wants to merge 1 commit into from

Conversation

ofrobots
Copy link
Contributor

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: #182

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
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 28, 2015
@ofrobots
Copy link
Contributor Author

@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.

@matthewloring
Copy link
Contributor

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.

@ofrobots
Copy link
Contributor Author

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.

@matthewloring
Copy link
Contributor

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.

@ofrobots
Copy link
Contributor Author

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.

@matthewloring
Copy link
Contributor

LGTM

@matthewloring
Copy link
Contributor

Landing in #190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants