denoland / deno_core

The core engine at the heart of Deno
MIT License
233 stars 76 forks source link

fix: Wake event loop when dynamic import promise resolves #769

Closed nathanwhit closed 1 month ago

nathanwhit commented 1 month ago

Fixes a hang that could occur when a module contained a dynamic import that doesn't resolve immediately, and there was a pending op outstanding.

The original reproduction (ref https://github.com/denoland/deno/issues/24098) effectively boiled down to this:

// main.ts
Deno.serve(async function (_req: Request): Promise<Response> {
    await import("./repro.ts");
    console.log("imported module");
    return new Response("done");
});

// repro.ts
await new Promise<void>((resolve) => setTimeout(resolve, 0));
console.log("module executed");

Upon getting a request, it would hang when trying to import "repro.ts". What was happening here was that Deno.serve created an op (op_http_serve) that stayed pending indefinitely, so we assumed that we would poll again, but that didn't actually hold. Unlike non-dynamic module evaluation, we weren't waking the event loop for polling when the dynamic import evaluation promise resolved.

The fix here is to attaching callbacks to the dynamic import promise to wake the event loop when it resolves, similar to what we do for normal module evaluation.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.

Project coverage is 81.44%. Comparing base (0c7f83e) to head (9ca4c73). Report is 22 commits behind head on main.

Files Patch % Lines
core/modules/map.rs 76.47% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #769 +/- ## ======================================== Coverage 81.43% 81.44% ======================================== Files 97 97 Lines 23877 23987 +110 ======================================== + Hits 19445 19536 +91 - Misses 4432 4451 +19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.