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

Clear existing timeouts when an abort signal aborts #36

Merged
merged 1 commit into from
Jun 12, 2023

Conversation

ronami
Copy link
Contributor

@ronami ronami commented Jun 12, 2023

Summary

Currently, running the following code would show the error being logged, but the process would only exist after 5 seconds (the value being passed as milliseconds):

import pTimeout from "p-timeout";

const promise = new Promise(() => {});
const abortController = new AbortController();

const p = pTimeout(promise, {
	milliseconds: 5000,
	signal: abortController.signal,
});

p.then(() => console.log("done")).catch((e) => console.log("error", e));

setTimeout(() => {
	abortController.abort();
}, 1000);

This happens because the setTimeout call (see here) isn't cleared when a signal has been passed and then aborted. This ongoing timeout can cause Node to hang until it's finished (if it's the only thing holding it from closing).

This PR fixes this by cleaning up the timers not only after the promise has finished, but after p-timeout's own promise finishes, which includes possibly rejecting because of an abort signal.

@ronami
Copy link
Contributor Author

ronami commented Jun 12, 2023

The main branch is currently red on what seems like an unrelated issue, see #37.

@ronami ronami force-pushed the fix-abort-cleanup branch from 5fef9a8 to 9052341 Compare June 12, 2023 14:10
@sindresorhus sindresorhus merged commit 52d1a6d into sindresorhus:main Jun 12, 2023
@ronami ronami deleted the fix-abort-cleanup branch June 12, 2023 22:30
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.

2 participants