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

Fix potentially undefined httpModule #4037

Merged
merged 2 commits into from Oct 11, 2021

Conversation

deammer
Copy link
Contributor

@deammer deammer commented Oct 7, 2021

Fixes #4036

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@kamilogorek
Copy link
Contributor

Can you share a stacktrace of the error? I'd like to understand how it can happen in the first place.

@deammer
Copy link
Contributor Author

deammer commented Oct 8, 2021

Hey @kamilogorek, I posted the stacktrace in #4036. I don't know if anything below the second line is very helpful though. For what it's worth, the issue occurred 100% of the time after we upgraded to 6.13.2, and it broke our product for 2 hours. After we went back to 6.9.0, things worked perfectly again.

@kamilogorek
Copy link
Contributor

I will merge this, as it's a trivial fix, however, we should better understand what's going on here.
Webckets (which this issue is related to) do not have a default agent so I'd understand if it would break on globalAgent.protocol access, but in this case, there are some API overrides shenanigans going on.

this should always refer to the http/s module, and we know for sure it exists, because when we wrap it, we do check for the method existence first in fill function:

const httpModule = require('http');
fill(httpModule, 'get', wrappedHandlerMaker);
fill(httpModule, 'request', wrappedHandlerMaker);

To fill it, it has to exist in the first place.

Is there a chance that you could somehow isolate this issue so we can correctly test it and not break like that in the future?

@kamilogorek kamilogorek merged commit 272043d into getsentry:master Oct 11, 2021
@deammer
Copy link
Contributor Author

deammer commented Oct 11, 2021

Thank you for merging the fix @kamilogorek. I've added a task to our backlog to isolate this issue and get back to you.

@deammer deammer deleted the bugfix/undefined-http-module branch October 11, 2021 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read property 'globalAgent' of undefined
2 participants