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.

Sign up for GitHub

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

Jump to bottom

AbortSignal.timeout() causes Deno.test() to report leaking resources? #20663

Closed
exside opened this issue Sep 24, 2023 · 1 comment · Fixed by #23842
Closed

AbortSignal.timeout() causes Deno.test() to report leaking resources? #20663

exside opened this issue Sep 24, 2023 · 1 comment · Fixed by #23842
Labels
bug Something isn't working correctly testing related to deno test and coverage

Comments

@exside
Copy link
Contributor

exside commented Sep 24, 2023 •
edited
Loading

I'm using AbortSignal.timeout() in fetch requests init object, when testing my concurrent request plugin i discovered that my tests fail, not because they don't work, but because deno is saying:

error: AssertionError: Test case is leaking async ops.

 - 1 async operation to sleep for a duration was started in this test, but never completed. This is often caused by not cancelling a `setTimeout` or `setInterval` call.

I mean it's not saying it explicitely, but my strong suspicion is that the automatic timeout() on the AbortSignal is what causes this...the problem is, how would i fix that? AFAIK there is no way to stop that timeout like we can with clearTimeout on regular setTimeouts, or didn't I dig deep enough into MDN?

EDIT: This is what is reported additionally when --trace-ops is added:

The operation was started here:
    at handleOpCallTracing (internal:core/01_core.js:237:42)
    at Object.op_sleep (eval at genAsyncOp (internal:core/01_core.js:181:14), <anonymous>:15:21)
    at Object.opAsync (internal:core/01_core.js:249:12)
    at runAfterTimeout (internal:ext/web/02_timers.js:219:31)
    at initializeTimer (internal:ext/web/02_timers.js:185:5)
    at setTimeout (internal:ext/web/02_timers.js:319:12)
    at Function.timeout (internal:ext/web/03_abort_signal.js:50:25)

so it looks like the suspicion was correct... well, now what can we do to fix this?

as far as a workaround disabling sanitizeResources in the tests probably will do, but i like that sanitizer and would prefer keeping it active (for other leaky stuff), so if there is a fix or other workaround (other than waiting for the entire timeout at the end of the test of course, lol) it would be good to know.

EDIT 2: Turns out i need to set both of those to false to make the actual tests pass without being the sanitizers the reason they fail:

	sanitizeResources: false,
	sanitizeOps: false,
@bartlomieju bartlomieju added bug Something isn't working correctly testing related to deno test and coverage labels Sep 24, 2023
@totto2727
Copy link

Although tentative, I was able to work around the problem by applying the following stubs.
However, since we cannot reproduce the timeout behavior, some tests may have problems.

export function abortSignalStub(): Stub<
  typeof AbortSignal,
  Parameters<typeof AbortSignal.timeout>,
  AbortSignal
> {
  return stub(
    AbortSignal,
    "timeout",
    () => new AbortController().signal,
  )
}

tdb-alcorn added a commit to tdb-alcorn/deno that referenced this issue May 16, 2024
@tdb-alcorn
Clear timeout in abort signal to fix leak denoland#20663
f9dfe4c
@tdb-alcorn tdb-alcorn mentioned this issue May 16, 2024
Merged
iuioiua added a commit that referenced this issue Jun 18, 2024
@tdb-alcorn @iuioiua @littledivy
fix(ext/web): fix AbortSignal.timeout() leak (#23842)
5289c69 
<!--
Before submitting a PR, please read
https://docs.deno.com/runtime/manual/references/contributing

1. Give the PR a descriptive title.

  Examples of good title:
    - fix(std/http): Fix race condition in server
    - docs(console): Update docstrings
    - feat(doc): Handle nested reexports

  Examples of bad title:
    - fix #7123
    - update docs
    - fix bugs

2. Ensure there is a related issue and it is referenced in the PR text.
3. Ensure there are tests that cover the changes.
4. Ensure `cargo test` passes.
5. Ensure `./tools/format.js` passes without changing files.
6. Ensure `./tools/lint.js` passes.
7. Open as a draft PR if your work is still in progress. The CI won't
run
   all steps, but you can add '[ci]' to a commit message to force it to.
8. If you would like to run the benchmarks on the CI, add the 'ci-bench'
label.
-->

Fixes #20663.

---------

Co-authored-by: Asher Gomez <[email protected]>
Co-authored-by: Divy Srivastava <[email protected]>
bartlomieju pushed a commit that referenced this issue Jun 18, 2024
@tdb-alcorn @iuioiua
@littledivy @bartlomieju
fix(ext/web): fix AbortSignal.timeout() leak (#23842)
9f7b008 
<!--
Before submitting a PR, please read
https://docs.deno.com/runtime/manual/references/contributing

1. Give the PR a descriptive title.

  Examples of good title:
    - fix(std/http): Fix race condition in server
    - docs(console): Update docstrings
    - feat(doc): Handle nested reexports

  Examples of bad title:
    - fix #7123
    - update docs
    - fix bugs

2. Ensure there is a related issue and it is referenced in the PR text.
3. Ensure there are tests that cover the changes.
4. Ensure `cargo test` passes.
5. Ensure `./tools/format.js` passes without changing files.
6. Ensure `./tools/lint.js` passes.
7. Open as a draft PR if your work is still in progress. The CI won't
run
   all steps, but you can add '[ci]' to a commit message to force it to.
8. If you would like to run the benchmarks on the CI, add the 'ci-bench'
label.
-->

Fixes #20663.

---------

Co-authored-by: Asher Gomez <[email protected]>
Co-authored-by: Divy Srivastava <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working correctly testing related to deno test and coverage
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(ext/web): fix AbortSignal.timeout() leak tdb-alcorn/deno
4 participants
@exside @lucacasonato @bartlomieju @totto2727