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

[Miniflare] mf.dispose() should internally await mf.ready #4363

Closed
RamIdeas opened this issue Sep 8, 2023 · 1 comment · Fixed by cloudflare/miniflare#705 or #4397
Closed

[Miniflare] mf.dispose() should internally await mf.ready #4363

RamIdeas opened this issue Sep 8, 2023 · 1 comment · Fixed by cloudflare/miniflare#705 or #4397
Labels
miniflare Relating to Miniflare

Comments

@RamIdeas
Copy link
Contributor

RamIdeas commented Sep 8, 2023

Calling mf.dispose() before the mf.ready promise has resolved causes this error:

Uncaught AssertionError [ERR_ASSERTION]: false == true
    at #waitForReady (.../node_modules/miniflare/dist/src/index.js:9402:33)
    at async Miniflare.dispose (.../node_modules/miniflare/dist/src/index.js:9463:7) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

Repro:

const mf = new miniflare.Miniflare({
  name: 'my-worker',
  modules: true,
  script: 'export default { fetch: req => new Response(req.url) }'
});

await mf.dispose();

Workaround:

const mf = new miniflare.Miniflare({
  name: 'my-worker',
  modules: true,
  script: 'export default { fetch: req => new Response(req.url) }'
});

await mf.ready;
await mf.dispose();

Ideally, the dispose method would internally await the ready promise

mrbbot referenced this issue in cloudflare/miniflare Oct 5, 2023
We previously waited for Miniflare to be ready before `dispose()`ing.
Unfortunately, we weren't waiting for the `workerd` config to finish
being written to stdin. Calling `dispose()` immediately after
`new Miniflare()` would stop waiting for socket ports to be reported,
and kill the `workerd` process while data was still being written.
This threw an unhandled `EPIPE` error.

This changes makes sure we don't report that Miniflare is ready until
after the config is fully-written.

Closes #680
mrbbot referenced this issue in cloudflare/miniflare Oct 5, 2023
We previously waited for Miniflare to be ready before `dispose()`ing.
Unfortunately, we weren't waiting for the `workerd` config to finish
being written to stdin. Calling `dispose()` immediately after
`new Miniflare()` would stop waiting for socket ports to be reported,
and kill the `workerd` process while data was still being written.
This threw an unhandled `EPIPE` error.

This changes makes sure we don't report that Miniflare is ready until
after the config is fully-written.

Closes #680
@RamIdeas
Copy link
Contributor Author

This is essentially fixed in cloudflare/miniflare#705 but I noticed my workaround to await mf.ready before await mf.dispose() stopped working. It seems like the ready promise returned before calling dispose never completes – I imagine it should reject?

@mrbbot mrbbot changed the title mf.dispose() should internally await mf.ready [Miniflare] mf.dispose() should internally await mf.ready Nov 7, 2023
@mrbbot mrbbot transferred this issue from cloudflare/miniflare Nov 7, 2023
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Nov 7, 2023
@mrbbot mrbbot added the miniflare Relating to Miniflare label Nov 7, 2023
@mrbbot mrbbot moved this from Untriaged to Backlog in workers-sdk Nov 7, 2023
@mrbbot mrbbot moved this from Backlog to In Review in workers-sdk Nov 8, 2023
@github-project-automation github-project-automation bot moved this from In Review to Done in workers-sdk Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
miniflare Relating to Miniflare
Projects
None yet
2 participants