Skip to content

Add per-worker proxy thread primitive - #1364

Merged
suketa merged 2 commits into
suketa:mainfrom
otegami:feature/worker-proxy-lifecycle
Jun 3, 2026
Merged

Add per-worker proxy thread primitive#1364
suketa merged 2 commits into
suketa:mainfrom
otegami:feature/worker-proxy-lifecycle

Conversation

@otegami

@otegami otegami commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

I did my best to minimize the diff, but it grew larger than I intended.
Apologies for the size of this change.

GitHub: GH-1136

This PR adds only the lifecycle primitive that a worker_proxy you can create
and destroy. There is no consumer yet, so it is behavior-preserving dead code
until the dispatch wiring and the scalar/table integrations is merged on top.

For the overall picture I have in mind — the full mechanism with every step
integrated — Please see otegami#7 on my fork.

What it adds

  • struct worker_proxy plus rbduckdb_worker_proxy_create / _destroy.
    destroy's signature matches duckdb_delete_callback_t so DuckDB can call it
    directly later.
  • The proxy thread loop and a GC-protection array, so proxy threads spawned from
    non-Ruby init hooks aren't collected mid-flight.

For reviewer's information

  • destroy releases the GVL while joining only when the caller holds it,
    because the proxy must run Ruby code (leaving the GC array) before it exits.
  • The proxy loop runs under rb_ensure, so teardown still signals completion if
    an async exception (Thread#kill, VM shutdown) unwinds it — otherwise the
    join deadlocks.
  • calloc/free, not xcalloc/xfree, since destroy may run off a non-Ruby
    thread.gi

Summary by CodeRabbit

  • Performance Improvements
    • Enhanced callback handling for user-defined functions, improving throughput and scalability when running concurrent DuckDB operations.

Introduce a per-worker proxy: one dedicated Ruby thread per DuckDB worker
thread, using the same mutex/condvar hand-off protocol as the global
executor but private to a single worker.

This commit lands only the lifecycle -- rbduckdb_worker_proxy_create and
rbduckdb_worker_proxy_destroy, plus the proxy thread loop and the
GC-protection array. The dispatch wiring that feeds requests to a proxy
follows in a separate commit, so these symbols are unused for now (public
linkage, no warning) and the full suite is unchanged.

worker_proxy_destroy releases the GVL while joining the proxy thread when
the caller holds it, since the proxy must run Ruby code (removing itself
from the GC array) before exiting -- otherwise teardown from a Ruby
thread would deadlock.
@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 10357859-b8ea-46f1-9efb-3c626c785d3d

📥 Commits

Reviewing files that changed from the base of the PR and between f41f9d4 and 16bfefc.

📒 Files selected for processing (2)
  • ext/duckdb/function_executor.c
  • ext/duckdb/function_executor.h

📝 Walkthrough

Walkthrough

This PR adds per-worker proxy threads to ruby-duckdb that allow DuckDB worker threads to dispatch UDF callbacks onto dedicated Ruby threads without serializing through a global queue. The implementation includes GC-safe thread lifecycle management, platform-specific synchronization, and proper teardown logic.

Changes

Per-worker proxy thread infrastructure

Layer / File(s) Summary
Public API and type declarations
ext/duckdb/function_executor.h
Header declares opaque worker_proxy handle and exports rbduckdb_worker_proxy_create() (GVL-held) and rbduckdb_worker_proxy_destroy() (non-Ruby-thread-safe) functions for proxy lifecycle management.
GC protection for proxy threads
ext/duckdb/function_executor.c
Static g_proxy_threads array is declared and lazily initialized as a global Ruby array inside rbduckdb_function_executor_ensure_started() to prevent proxy Ruby thread objects from being garbage-collected while DuckDB may still dispatch callbacks to them.
Per-worker proxy struct and main loop
ext/duckdb/function_executor.c
struct worker_proxy contains callback function pointer/user data, request/stop/completion flags, and platform-specific synchronization (pthreads mutex/condvars on Unix, critical sections/events on Windows); the proxy loop releases the GVL while waiting, then reacquires it to invoke the callback, signals completion, and includes rb_ensure-based cleanup that deregisters from GC protection and sets thread-exit state.
Proxy destruction with GVL safety
ext/duckdb/function_executor.c
rbduckdb_worker_proxy_destroy() signals shutdown, joins the proxy thread until full exit (conditionally releasing the GVL to prevent deadlock if called from a Ruby thread), destroys the platform-specific synchronization primitives, and frees the proxy struct.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • suketa/ruby-duckdb#1257: Extracts the single global executor thread and dispatcher infrastructure that this PR builds upon to implement per-worker proxy threads.
  • suketa/ruby-duckdb#1280: Routes table function bind/init/execute callbacks through the executor infrastructure; this PR extends the executor with per-worker proxies to handle such dispatched callbacks safely.
  • suketa/ruby-duckdb#1140: Implements callback dispatch from non-Ruby worker threads onto Ruby-managed executor threads; this PR generalizes that pattern to per-worker proxies instead of a single global queue.

Suggested reviewers

  • suketa

🐰 With threads now dancing per-worker, no queue will block—
Each proxy spins its Ruby loop, callbacks unlock.
GC guards them safe from the garbage's might,
While GVL waltzes in and out of sight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately describes the main change: adding a per-worker proxy thread primitive with creation/destruction APIs.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@suketa suketa left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM, thank you.

@suketa
suketa merged commit 5b2f10b into suketa:main Jun 3, 2026
35 checks passed
@otegami
otegami deleted the feature/worker-proxy-lifecycle branch June 3, 2026 23:44
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.

2 participants