cleanup containers directly instead of using process.exit hook #10986
Conversation
🦋 Changeset detectedLatest commit: 13e2816 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
emily-shen
force-pushed
the
emily/fix-container-exit
branch
from
2df5f63 to
b59adfb
Compare
| #teardown = async (): Promise<void> => { | ||
| logger.debug("MultiworkerRuntimeController teardown beginning..."); | ||
|
|
||
| this.cleanupContainers(); |
There was a problem hiding this comment.
Is this needed since the public teardown() method calls super, which will run the cleanup already?
| }, WAITFOR_OPTIONS); | ||
|
|
||
| wrangler.pty.kill("SIGINT"); | ||
| // ctrl + c |
There was a problem hiding this comment.
weirdly sending a sigint to the wrangler process in interactive-dev-tests was always cleaning up wrangler containers (so we didn't catch the regression), but writing ctrl+c does fail without the changes in this pr
2 participants
Wrangler dev regressed a bit and stopped removing containers on exit after #10862.
This PR also adds a direct call to cleanupContainers in the teardown hook.
I think removing and re adding event listeners in onBundleStart should still stop the tests flaking still.
We can't remove the process.exit hooks entirely, since if we don't manage to call teardown (e.g. when something like concurrently is killing wrangler), we won't cleanup containers.