Skip to content

some work on supporting piscina - #19940

Closed
alii wants to merge 186 commits into
mainfrom
ali/piscina
Closed

alii wants to merge 186 commits into
mainfrom
ali/piscina

Conversation

@alii

@alii alii commented May 27, 2025 •

Copy link
Copy Markdown
Member

What does this PR do?

Fixes #19924
Fixes #13218

CleanShot 2025-05-27 at 14 58 41@2x

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

third party test (but will likely need to do more since there are worker related changes)

@alii
alii marked this pull request as draft May 27, 2025 21:56
@robobun

robobun commented May 27, 2025 •

Copy link
Copy Markdown
Collaborator
Updated 4:21 PM PT - Oct 20th, 2025

❌ @alii, your commit a45c263 has 1 failures in Build #29763 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 19940

That installs a local version of the PR into your bun-19940 executable, so you can run:

bun-19940 --bun

Comment thread src/bun.js/bindings/webcore/Worker.cpp Outdated
Comment thread src/bun.js/bindings/webcore/MessagePort.cpp Outdated
Comment thread .vscode/settings.json Outdated
Comment thread src/bun.js/web_worker.zig Outdated
@alii
alii requested review from 190n, Jarred-Sumner and heimskr May 31, 2025 00:26
Comment thread src/bun.js/bindings/webcore/Worker.cpp Outdated
Comment thread src/bun.js/bindings/webcore/MessagePortChannelRegistry.cpp Outdated
Comment thread src/bun.js/bindings/webcore/MessagePort.cpp Outdated
Comment thread src/bun.js/bindings/webcore/MessagePort.cpp
Comment thread src/bun.js/bindings/webcore/MessagePort.cpp Outdated
Comment thread src/bun.js/bindings/webcore/MessagePort.cpp Outdated
Comment thread src/bun.js/event_loop.zig Outdated
Comment thread src/bun.js/bindings/ScriptExecutionContext.h Outdated
Comment thread src/bun.js/bindings/ScriptExecutionContext.cpp Outdated
Comment thread src/bun.js/web_worker.zig Outdated
alii added a commit that referenced this pull request May 31, 2025
Comment thread src/bun.js/bindings/ScriptExecutionContext.cpp Outdated

@heimskr heimskr left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my comments are nitpicks, but the use of reinterpret_cast instead of static_cast for casting JSC::JSGlobalObject* to Zig::GlobalObject* is a safety issue. If there's any doubt whatsoever about whether a JSC::JSGlobalObject is also an instance of Zig::GlobalObject, use jsCast for an assertion failure (including in release builds) or jsDynamicCast for returning nullptr if the cast fails. Otherwise, static_cast is fine.

Comment thread src/bun.js/bindings/ScriptExecutionContext.cpp Outdated
Comment thread src/bun.js/bindings/ScriptExecutionContext.cpp Outdated
Comment thread src/bun.js/bindings/ScriptExecutionContext.cpp Outdated
Comment thread src/bun.js/bindings/ScriptExecutionContext.h Outdated
Comment thread src/bun.js/bindings/webcore/MessagePort.cpp Outdated
Comment thread src/bun.js/event_loop.zig Outdated
Comment thread src/bun.js/web_worker.zig Outdated
Comment thread src/bun.js/web_worker.zig
Comment thread src/bun.js/web_worker.zig Outdated
Comment thread src/bun.js/web_worker.zig Outdated
Comment thread src/bun.js/web_worker.zig Outdated
Comment thread src/bun.js/bindings/webcore/Worker.cpp Outdated
Comment thread vibe-tools.config.json Outdated
Comment thread test/js/third_party/astro/astro-post.test.js
@190n

190n commented Jun 6, 2025

Copy link
Copy Markdown
Contributor

This assertion failure in test-worker-message-port-transfer-terminate is interesting: https://buildkite.com/bun/bun/builds/18199#01974255-d25e-4706-8891-290500bfd29b/2013-2014

Trying to repro

@190n 190n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments + I'll figure out the assertion failure

Comment thread scripts/runner.node.mjs Outdated
const flakyResults = [];
const failedResults = [];
const maxAttempts = 1 + (parseInt(options["retries"]) || 0);
const defaultMaxAttempts = 1 + (parseInt(options["retries"]) || 0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should revert this before merge

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg

Comment thread test/js/web/workers/worker-terminate.test.mjs Outdated
Comment thread test/js/web/workers/worker-memory-leak.test.ts Outdated
Comment thread test/js/third_party/piscina/piscina.test.ts
Comment thread test/js/node/worker_threads/worker-with-process-exit-delay-and-terminate.mjs Outdated
Comment thread src/bun.js/web_worker.zig Outdated
Comment thread src/bun.js/web_worker.zig
Comment thread src/bun.js/web_worker.zig Outdated
Comment thread src/bun.js/event_loop/README.md Outdated
Comment thread src/bun.js/event_loop.zig Outdated
@alii

alii commented Jan 5, 2026

Copy link
Copy Markdown
Member Author

Closing as:

  • this PR grew too big
  • many of the fixes it implemented already merged elsewhere, or were fixed in other ways
  • conflicts

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.

Support piscina Support Module.prototype._compile

7 participants