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(async-utils): prevent timeout and interval checks in wait from leaving open handles #682

Merged
merged 3 commits into from
Aug 31, 2021

Conversation

mpeyper
Copy link
Member

@mpeyper mpeyper commented Aug 30, 2021

What:

This issue was raised in discord but the use of Promise.race has also been flagged as potentially an issue with fake timers in #631.

(I'm going to use "timeout" a lot in here that had slightly different meanings so I hope it doesn't get too confusing).

The timeout that get created to stop waiting after the async utils timeout is exceeded and the timeout that created for running intverval checks when the hook doesn't rerender fast enough are not being cleared when the wait condition is being satisfied and the promise resolves before they fire. This is leaving open handles which jest is warning about.

Interestingly, the warning only appears when the testEnvironment is not set to jsdom in the jest config (like in our test suite), which I haven't been able to properly identify why, but I'm assuming jsdom must be intercepting these handle somehow and cleaning them up, which doesn't occur when using the node test environment.

Why:

We want to be good citizens and clean up after outselves.

How:

The Promise.race was not the actual cause, but the use of racing promises and letting the losing promise just run to completion was not helping here. While the implementation allowed for each part to remain relatively simple, it made it difficult to cancel the loser when the winner resolved.

The new implementation introduces a timeoutSignal that controls the timeouts and wraps the promise in a way that is aware of when it completely as clears the pending timeouts so they are no longer left open. This means there is a little bit more ceremony in using it and we need to check the whether it timed out in a few more places to ensure things exit cleanly.

Checklist:

  • Documentation updated
    • N/A
  • Tests
    • The existing tests are passing without modification, but I'm unsure if we can/want to run specific tests in a non-jsdom test environment with --detectOpenHandles to ensure it stays working (I don't particularly want to introduce any more variations of the test suite)
  • Ready to be merged
  • Added myself to contributors table

@netlify
Copy link

netlify bot commented Aug 30, 2021

✔️ Deploy Preview for react-hooks-testing-library ready!

🔨 Explore the source changes: 4a6a8d5

🔍 Inspect the deploy log: https://app.netlify.com/sites/react-hooks-testing-library/deploys/612e10bac0d37e000791a08a

😎 Browse the preview: https://deploy-preview-682--react-hooks-testing-library.netlify.app

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #682 (4a6a8d5) into main (b869640) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #682   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          235       244    +9     
  Branches        33        32    -1     
=========================================
+ Hits           235       244    +9     
Impacted Files Coverage Δ
src/core/asyncUtils.ts 100.00% <100.00%> (ø)
src/helpers/createTimeoutController.ts 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 b869640...4a6a8d5. Read the comment docs.

@joshuaellis
Copy link
Member

I think we're okay without the extra tests, I agree they'll add more complexity to our test suite, I think if we begin seeing this as a pain point, we can then address adding specific tests.

@mpeyper
Copy link
Member Author

mpeyper commented Aug 31, 2021

I just made a small change to rename createTimeoutSignal to createTimeoutController. To be honest, I'm not sure what to call it, but it felt like it was behaving more like the AbortController itself that the AbortController.signal that I originally named it after. The functionality hasn't changed at all, just the name.

@mpeyper mpeyper merged commit 4a03704 into main Aug 31, 2021
@mpeyper mpeyper deleted the fix/async-utils branch August 31, 2021 11:25
@github-actions
Copy link

🎉 This PR is included in version 7.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants