fix(rhai): run Rhai script hooks via spawn_blocking instead of the async executor#9815
fix(rhai): run Rhai script hooks via spawn_blocking instead of the async executor#9815rohan-b99 wants to merge 5 commits into
Conversation
…ync executor Rhai request/response hooks were evaluated inline on the Tokio async executor thread, so a CPU-bound script (loops, regex, etc.) held that thread for the full duration of its evaluation and delayed every other in-flight request scheduled on it. execute() now runs the script on Tokio's blocking thread pool via spawn_blocking, and the subgraph gen_map_response macro was switched from a synchronous map_response() to an async and_then() so it can await it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 14 changed, 0 removedBuild ID: 94cfaa4e3b1a87a5f64d34df URL: https://www.apollographql.com/docs/deploy-preview/94cfaa4e3b1a87a5f64d34df ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
|
/claude-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let ticked = after - before; | ||
| assert!( | ||
| ticked >= 8, | ||
| "the async ticker only progressed by {ticked} ticks while the rhai script executed; \ | ||
| the executor thread was blocked" | ||
| ); |
There was a problem hiding this comment.
This new regression test judges success by wall-clock timing rather than a condition-based wait, which matches two documented antipatterns in CLAUDE.md:
- Fixed sleeps instead of condition-based waits — the ticker is spawned and then a fixed
sleep(5ms)(line 281) is used to "let the ticker get going" before thebeforebaseline read. This is the "spawn + sleep N ms" readiness pattern; under CI scheduling jitter 5ms may not be enough for the ticker task to be polled even once, skewing the baseline. - Nanosecond-timer races in tests —
assert!(ticked >= 8, ...)decides the outcome by racing a 1ms-interval ticker against a 50ms busy-loop with an empirically-tuned threshold (the comment itself notes "observed 1-2" vs "observed 20+"). On a loaded runner the tick count can compress even when the fix is working correctly, producing a flaky failure.
Consider synchronizing on a condition rather than a fixed warm-up (e.g. spin until ticks becomes non-zero before capturing before). Worth noting too: this is a lib unit test (binary_id apollo-router), so it is not covered by the threads-required = 4 safeguard in .config/nextest.toml, which only applies to the apollo-router::* integration binaries where timing-sensitive tests are otherwise protected.
There was a problem hiding this comment.
yeah...this smells flaky but I'm also not totally sure how else to go about it.
goto-bus-stop
left a comment
goto-bus-stop
left a comment
There was a problem hiding this comment.
it looks good really. hard to predict the performance impact. some noncommittal comments below
| let response_opt = guard.take(); | ||
| response_opt.unwrap() | ||
| }) | ||
| .boxed_clone() |
There was a problem hiding this comment.
Why did this change to BoxCloneService::new?
There was a problem hiding this comment.
Looks like the other macros were using it - I've switched all 3 to use .boxed_clone for consistency
| let ticked = after - before; | ||
| assert!( | ||
| ticked >= 8, | ||
| "the async ticker only progressed by {ticked} ticks while the rhai script executed; \ | ||
| the executor thread was blocked" | ||
| ); |
There was a problem hiding this comment.
yeah...this smells flaky but I'm also not totally sure how else to go about it.
|
|
||
| let duration = start.elapsed(); | ||
| let (result, duration) = match tokio::task::spawn_blocking(move || { | ||
| let start = Instant::now(); |
There was a problem hiding this comment.
Honestly it might be better if the waiting time for the task being scheduled is included...or if we had a histogram for rhai wait time and one for execution time.
There was a problem hiding this comment.
I've moved the Instant::now call to before spawn_blocking, let me know if its worth raising a ticket to add a new metric though
2 participants
Rhai request/response hooks were evaluated inline on the Tokio async executor thread, so a CPU-bound script (loops, regex, etc.) held that thread for the full duration of its evaluation and delayed every other in-flight request scheduled on it. execute() now runs the script on Tokio's blocking thread pool via spawn_blocking, and the subgraph gen_map_response macro was switched from a synchronous map_response() to an async and_then() so it can await it.
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩