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: Do not use WeakRef on mocks. #1290

Merged
merged 3 commits into from Mar 18, 2022
Merged

fix: Do not use WeakRef on mocks. #1290

merged 3 commits into from Mar 18, 2022

Conversation

ShogunPanda
Copy link
Contributor

This PR fixes a very hard to spot bug that happens under these conditions:

  1. You use mocks and setup some interceptors.
  2. The pool/client (mockAgent.get) is setup within a function and no reference to the pool/client is returned.
  3. A lot of request (same or different url doesn't matter) are performed and a lot of data is returned.
  4. Some time passes (either with setTimeout or just with regular application I/O delays)

What happens is that the original pool/client is garbage collected and thus all the set interceptors are gone, leading to either test failure or unwanted network activity.

The PR fixes that by using a FakeWeakRef in the MockAgent that keeps a reference to the pool and thus no garbage collection happens.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2022

Codecov Report

Merging #1290 (b9419cf) into main (fc79429) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1290   +/-   ##
=======================================
  Coverage   94.14%   94.14%           
=======================================
  Files          44       44           
  Lines        4098     4099    +1     
=======================================
+ Hits         3858     3859    +1     
  Misses        240      240           
Impacted Files Coverage Δ
lib/mock/mock-agent.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc79429...b9419cf. Read the comment docs.

@ronag ronag merged commit d502c57 into nodejs:main Mar 18, 2022
@ShogunPanda ShogunPanda deleted the mock-fake-weak-ref branch March 18, 2022 11:08
KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
* fix: Do not use WeakRef on mocks.

* fix: Linting errors.

* fix: Fixed test on older versions of Node.
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
* fix: Do not use WeakRef on mocks.

* fix: Linting errors.

* fix: Fixed test on older versions of Node.
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* fix: Do not use WeakRef on mocks.

* fix: Linting errors.

* fix: Fixed test on older versions of Node.
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.

None yet

4 participants