fix(adapter-node-http): Remove module monkey patching on disconnect #369
Conversation
This file contains 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
woodenconsulting
force-pushed
the
master
branch
from
1276d54 to
472fd0c
Compare
woodenconsulting
changed the title
woodenconsulting
force-pushed
the
master
branch
from
472fd0c to
3d83e83
Compare
jasonmit
reviewed
Nov 18, 2020
| @@ -120,6 +121,11 @@ export default class HttpAdapter extends Adapter { | |||
| const module = modules[moduleName]; | |||
| const { request, get, globalAgent } = module; | |||
|
|
|||
| this[moduleName] = { | |||
There was a problem hiding this comment.
nit: I'll probably end up renaming this but don't feel like blocking the PR on it :)
|
Thanks for catching & fixing the memory leak - they're rarely fun to track down so much appreciated 🙏 |
|
Fixes #370 |
|
Released with |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Fixes a memory leak with Jest due to native module monkey patching. More details in the issue I filed here. Note that I did not add any tests; implementing tests to assert the existence of a memory leak is not simple, but the changes here are. This is easily demonstrated in any jest test suite using polly with the node-http adapter by running the following command:
Motivation and Context
Memory leaks are bad :)
Types of Changes
Checklist