-
Notifications
You must be signed in to change notification settings - Fork 751
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
[wrangler] fix: prevent zombie workerd
processes
#4635
Conversation
🦋 Changeset detectedLatest commit: 74bdc1d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7275199171/npm-package-wrangler-4635 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7275199171/npm-package-wrangler-4635 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7275199171/npm-package-wrangler-4635 dev path/to/script.js Additional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7275199171/npm-package-miniflare-4635 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7275199171/npm-package-cloudflare-pages-shared-4635 npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7275199171/npm-package-create-cloudflare-4635 --no-auto-update Note that these links will no longer work once the GitHub Actions artifact expires.
| Please ensure constraints are pinned, and |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4635 +/- ##
==========================================
+ Coverage 75.19% 75.22% +0.03%
==========================================
Files 242 242
Lines 13014 13014
Branches 3346 3346
==========================================
+ Hits 9786 9790 +4
+ Misses 3228 3224 -4
|
workerd
processesworkerd
processes
Fixes #4612.
Fixes #4131.
Fixes cloudflare/workers-rs#425.
What this PR solves / how to test:
Previously, running
wrangler dev
would leave behind "zombie"workerd
processes. These processes prevented the same port being bound ifwrangler dev
was restarted and sometimes consumed lots of CPU time. This PR ensures allworkerd
processes are killed whenwrangler dev
is shutdown.Massive thank you to @ezg27 for identifying that commenting out
await closeSentry()
fixes the issue. I'm still not quite sure why this is happening. This is what I've observed:SENTRY_DSN
environment variable is set during the build)wrangler-dist/cli.js
directly rather than through thebin/wrangler.js
bootstrapper fixes the issue, "ipc"
from...workers-sdk/packages/wrangler/bin/wrangler.js
Line 65 in a6a4e8a
await closeSentry()
andprocess.disconnect?.()
fixes the issueAuthor has addressed the following:
wrangler
viashellac
(likely not ashellac
problem), so we can't use our existing E2E testing framework to write tests for this. Given this is affecting lots of users, I'd prefer we get a fix out now, then investigate/test this later.Note for PR author:
We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label
highlight pr review
so future reviewers can take inspiration and learn from it.