feat: route the non-Ruby-thread dispatch path through an optional proxy - #1365
Conversation
Split the dispatcher so the worker-thread path (Case 3) can route through a per-worker proxy. rbduckdb_function_executor_dispatch_via_proxy is the new entry point: when proxy is non-NULL it hands the callback to that proxy (Case 3a), otherwise it falls back to the global executor (Case 3b). dispatch_callback_to_proxy is the sender half of the proxy hand-off -- the counterpart of the receiver loop that landed with the primitive. rbduckdb_function_executor_dispatch keeps its signature and now simply delegates with NULL, so behavior is unchanged -- the only in-tree caller passes NULL and no proxy is ever exercised yet. Full suite unchanged (1147). The proxy path goes live once scalar/table integration lands. Also document rbduckdb_worker_proxy_create's raise contract: the executor runs callbacks unprotected, so a wrapper that dispatches create through it must rb_protect the call. The wrapper itself arrives with the first consumer (scalar integration), which is where the rb_protect lands.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extends the DuckDB function executor with a per-worker proxy dispatch path for non-Ruby threads. It introduces a new exported dispatcher that routes callbacks either through a specified per-worker proxy, falls back to a global executor, or executes directly for Ruby threads with/without the GVL. ChangesPer-worker proxy dispatch extension
Sequence Diagram(Omitted—changes introduce a dispatch routing mechanism without a multi-component interaction flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)ext/duckdb/function_executor.cIn file included from ext/duckdb/function_executor.c:1: ... [truncated 752 characters] ... inux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" 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. Comment |
2 participants
GitHub: GH-1136
This is step 2 of the per-worker proxy work, on top of GH-1364. It wires the
dispatcher so the non-Ruby-thread path (Case 3) can optionally route through a
per-worker proxy instead of the global executor. Behavior is unchanged: the
only in-tree caller passes NULL, so the proxy path is not exercised yet. It
goes live with the scalar integration in the next PR.
For the overall picture I have in mind — the full mechanism with every step
integrated — see #7 on my fork: otegami#7
What it adds
rbduckdb_function_executor_dispatch_via_proxy(cb, user_data, proxy): thenew entry point. When
proxyis non-NULL, Case 3 hands the callback to thatproxy (Case 3a); when NULL, it falls back to the global executor (Case 3b).
Cases 1 and 2 are unchanged.
dispatch_callback_to_proxy: the sender half of the proxy hand-off — setsthe request under the proxy's lock, signals it, and blocks until done. This
is the counterpart of the receiver loop merged in Add per-worker proxy thread primitive #1364.
rbduckdb_function_executor_dispatchkeeps its signature and now delegateswith NULL, so existing callers are byte-for-byte on the old path.
For reviewer's information
rbduckdb_worker_proxy_createnow states its raisecontract: the executor runs callbacks unprotected, so a wrapper that
dispatches
createthrough it mustrb_protectthe call. The wrapperarrives with the first consumer (scalar integration), which is where the
rb_protectlands.Summary by CodeRabbit