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: make Application listen promise resolve after aborted #685

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Thesephi
Copy link

@Thesephi Thesephi commented Dec 3, 2024

Hi Oak community / cc @kitsonk,


Edited on 12th Dec 24: should close #686 and close #687


I'm using Oak heavily at work (kudos to all for the cool library of course 😀).

I notice (at current version v17.1.3) the server listen promise doesn't resolve itself even after already aborted.

Naive reproduction attempt:

const abortCtrl = new AbortController();
const { signal } = abortCtrl;
const app = new Application();
const p = app.listen({ signal });
setTimeout(() => abortCtrl.abort(), 2000); // simulate turning off the application after 2 seconds
await p;
console.log("application aborted, safe to shut down the parent process"); // <== unreachable line

If we do it like the above, then we never see the last line logged out in the console. This suggests the await p expression never resolves (forever staying at pending state).

I was a bit curious how it behaves differently from the doc, so I dug a bit & it resulted in this PR.

I leave the PR as draft so we can discuss more where needed.
update 6th Dec: I mark the PR as ready for visibility - always happy to consider alternatives or feedback :)


Side note:

During the process, I also noticed a related unit test was disabled. I think it was disabled due to another (slightly related) reason: the server needs a bit of time to initialize itself, so if we call abort() right after listen() (as done in the originally disabled test), then the signal is already aborted by the time the server initializes, and so we "miss" the chance to properly abort it.

To this end, I also re-enabled that test, plus I added another test so we cover both scenarios. The PR content explains better than my words here tho 😀


Thank you everyone for your insights / reviews / discussions / objections etc. 🙌 !

@CLAassistant
Copy link

CLAassistant commented Dec 3, 2024

CLA assistant check
All committers have signed the CLA.

@Thesephi
Copy link
Author

Thesephi commented Dec 3, 2024

PS: apologies @kitsonk, I forgot deno fmt . Should be fixed at the latest PR commit :)

@Thesephi Thesephi marked this pull request as ready for review December 6, 2024 16:11
@nomocas nomocas mentioned this pull request Dec 10, 2024
@nomocas
Copy link

nomocas commented Dec 11, 2024

Hey @Thesephi,

If you have the rights to do so, don't hesitate to reference my issue in the "development" section in the right column above.
Something as "close #686". It will close the issue automatically when this PR is merged.

Thank you!

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.

Abort sequence issue
3 participants