Fix async props race conditions #2297
Conversation
Implements lazy initialization pattern for AsyncPropsManager to handle race conditions between initial render requests and update chunks. The new getOrCreateAsyncPropsManager function: - Accepts sharedExecutionContext as parameter - Creates or retrieves existing AsyncPropsManager instance - Ensures same manager is used regardless of which code executes first This fixes the bug where commenting out sleep statements in test_incremental_rendering.html.erb caused async props not to be sent, because update chunks were arriving before the manager was set up. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove unused log import (replaced by console.log for debugging) - Change ++ operators to += 1 to satisfy no-plusplus rule Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Code Review: Fix async props race condition with lazy AsyncPropsManager initializationSummaryThis PR effectively addresses the race condition described in #2270 by implementing lazy initialization of ✅ Strengths1. Excellent Architecture & Documentation
2. Clean Lazy Initialization Pattern
3. Minimal API Surface Changes
4. Good Error Handling
|
size-limit report 📦
|
AbanoubGhadban
force-pushed
the
fix/getOrCreateAsyncPropsManager-lazy-init
branch
from
5efcf80 to
455e7d4
Compare
Greptile OverviewGreptile SummaryThis PR successfully addresses a race condition in async props handling where update chunks could arrive before Key ChangesCore Fix:
Architecture: Issues FoundCritical Issue: Code Quality:
Race Condition ResolutionThe lazy initialization pattern correctly handles all timing scenarios:
The implementation is thread-safe because Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Rails as Rails Server
participant Worker as Node Worker
participant VM as VM Context
participant Manager as AsyncPropsManager
participant React as React Component
Note over Rails,React: Race Condition Fix: Lazy Initialization Pattern
Rails->>Worker: POST /incremental-render<br/>(First NDJSON line: renderingRequest)
Worker->>VM: handleIncrementalRenderRequest()
VM->>VM: buildExecutionContext()<br/>Creates sharedExecutionContext Map
Note over VM,Manager: INITIAL RENDER PATH
VM->>VM: Execute renderingRequest JS
VM->>Manager: getOrCreateAsyncPropsManager(sharedExecutionContext)
alt Manager doesn't exist
Manager->>Manager: new AsyncPropsManager()
Manager->>VM: Store in sharedExecutionContext["asyncPropsManager"]
end
VM->>React: addAsyncPropsCapabilityToComponentProps(props, sharedExecutionContext)
React->>Manager: getReactOnRailsAsyncProp("users")
Manager->>React: Returns Promise (suspends)
React-->>Worker: Initial HTML shell
par Update Chunks Can Arrive Anytime
Note over Rails,Manager: UPDATE CHUNK PATH (can race with initial render)
Rails->>Worker: NDJSON line 2: updateChunk for "users"
Worker->>VM: sink.add(updateChunk)
VM->>VM: Execute updateChunk JS
VM->>Manager: getOrCreateAsyncPropsManager(sharedExecutionContext)
alt Manager already exists
Manager->>Manager: Return existing instance
else Manager doesn't exist yet (race condition)
Manager->>Manager: new AsyncPropsManager()
Manager->>VM: Store in sharedExecutionContext["asyncPropsManager"]
end
VM->>Manager: setProp("users", userData)
Manager->>React: Resolve Promise
React-->>Worker: More HTML chunks
end
Rails->>Worker: Close HTTP request
Worker->>VM: sink.handleRequestClosed()
VM->>Manager: getOrCreateAsyncPropsManager(sharedExecutionContext)
VM->>Manager: endStream()
Manager->>Manager: Reject unresolved promises
React-->>Worker: Final HTML chunks
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
Old (what this fixture uses): var { props: propsWithAsyncProps, asyncPropManager } =
ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps);New (what it should be): var { props: propsWithAsyncProps } =
ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps, sharedExecutionContext);The new signature:
This fixture needs to be updated to match the new API, otherwise tests using this fixture will fail. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js
Line: 23:25
Comment:
This test fixture uses the OLD API signature for `addAsyncPropsCapabilityToComponentProps`, which is incompatible with the changes in this PR. The function signature has changed:
**Old (what this fixture uses):**
```javascript
var { props: propsWithAsyncProps, asyncPropManager } =
ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps);
```
**New (what it should be):**
```javascript
var { props: propsWithAsyncProps } =
ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps, sharedExecutionContext);
```
The new signature:
1. Requires `sharedExecutionContext` as a second parameter
2. No longer returns `asyncPropManager` (it's managed internally via `getOrCreateAsyncPropsManager`)
3. The manual `sharedExecutionContext.set("asyncPropsManager", asyncPropManager)` line is no longer needed
```suggestion
if (ReactOnRails.isRSCBundle) {
var { props: propsWithAsyncProps } = ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps, sharedExecutionContext);
usedProps = propsWithAsyncProps;
}
```
This fixture needs to be updated to match the new API, otherwise tests using this fixture will fail.
How can I resolve this? If you propose a fix, please make it concise. |
Code Review - PR #2297SummaryThis PR fixes a race condition in async props handling by implementing lazy initialization of the AsyncPropsManager. The core issue is that update chunks could arrive before the initial render creates the manager, causing async props to be lost. ✅ Strengths1. Solid Architecture - Lazy Initialization PatternThe
2. Excellent DocumentationThe JSDoc comments clearly explain:
This is particularly well done in 3. Type SafetyProper TypeScript types added to 4. Backward CompatibilityThe change is internal - no breaking changes to the public API.
|
Remove debugging logs from: - AsyncPropsManager.ts - handleIncrementalRenderRequest.ts - handleIncrementalRenderStream.ts Co-Authored-By: Claude Opus 4.5 <[email protected]>
AbanoubGhadban
force-pushed
the
fix/getOrCreateAsyncPropsManager-lazy-init
branch
from
455e7d4 to
30c296d
Compare
Code Review - PR #2297This PR successfully addresses a race condition in async props delivery by implementing lazy initialization of AsyncPropsManager. The fix is well-architected and follows good engineering principles. Strengths
Critical Issue: Missing Test CoverageThe existing test (test_incremental_rendering.html.erb) uses sleep 1 which MASKS the race condition this PR fixes. You need a test that exercises the race condition without sleep delays. Please add a test that emits props immediately to verify the fix prevents regression. Other Suggestions
VerdictApproved with suggestions. Core fix is solid, but please add the automated test before merging. Code Quality: 8.5/10 Great work identifying and fixing this subtle timing issue! |
Code Review - PR 2297Overall AssessmentStrong fix for a critical race condition. The implementation is well-thought-out and follows good engineering practices. Strengths
Issues and RecommendationsCRITICAL: Missing Test CoverageThe PR has NO tests for the race condition fix. This is a serious gap for a concurrency issue. Required tests:
The test plan checkbox in the PR description is unfilled. This needs local testing before merge per CLAUDE.md guidelines. MODERATE: RSpec Test Needs UpdateThe existing test at react_on_rails_pro/spec/react_on_rails_pro/server_rendering_js_code_spec.rb lines 38-39 and 84 checks for the OLD implementation and will likely fail because asyncPropManager variable no longer exists in generated JS. Security and Performance
Before Merging Checklist
ConclusionThis is a solid fix for a real production issue, but it needs test coverage before merging. The race condition is subtle and could easily regress without tests. Recommendation: Request changes for test coverage, then approve once tests are added. Great work identifying and solving this timing issue! |
Async props requires RSC bundle because getOrCreateAsyncPropsManager is only implemented there (server bundle has a stub that throws). Co-Authored-By: Claude Opus 4.5 <[email protected]>
Code Review FeedbackOverall AssessmentThis PR effectively solves a critical race condition in async props handling. The lazy initialization pattern is a solid architectural choice that properly addresses the core issue where update chunks could arrive before the AsyncPropsManager was initialized. Strengths
CRITICAL Issues Found1. Outdated Test Assertion File: react_on_rails_pro/spec/react_on_rails_pro/async_props_emitter_spec.rb:23 The test on line 23 still expects the OLD code pattern:
But the implementation NOW uses:
This test will FAIL! It needs to be updated to expect the new function call pattern. 2. Missing Test Coverage The PR test plan mentions testing stream_react_component_with_async_props without sleep delays, but:
Recommendation: Add an integration test that calls stream_react_component_with_async_props with NO sleep delays and verifies async props are delivered correctly. 3. Type Safety Issue File: packages/react-on-rails-pro/src/AsyncPropsManager.ts:158 The type assertion assumes the value is either AsyncPropsManager or undefined, but Map.get() could return ANY value if something else pollutes the key. Consider adding an instanceof check for defensive programming against accidental key collisions. Security & PerformanceSecurity:
Performance:
Required Actions Before Merge
Status: Needs work - Fix the test assertion before merging. Otherwise, solid implementation of the race condition fix. Generated with Claude Code |
Code Review SummaryThis PR effectively fixes the race condition in async props delivery by implementing lazy initialization of AsyncPropsManager. The approach is sound and well-documented. Strengths
Potential Issues & Recommendations1. Test Coverage Gap (HIGH PRIORITY)The existing test file (AsyncPropManager.test.ts) only tests the AsyncPropsManager class directly, but doesn't test the new getOrCreateAsyncPropsManager() function or the race condition scenario it solves. Recommendation: Add tests that verify multiple calls return the same instance and that update chunks arriving before initial render work correctly. 2. RSpec Test Update Required (HIGH PRIORITY)The spec file async_props_emitter_spec.rb:23 still checks for the old pattern sharedExecutionContext.get but the new code uses ReactOnRails.getOrCreateAsyncPropsManager(sharedExecutionContext). This test will likely fail. Please update the expectation to match the new implementation. 3. Minor: Improved Error MessageThe error message improvement in async_props_emitter.rb:37-38 is great, but consider making backtrace formatting more defensive to avoid nil values. 4. Documentation: Bundle Hash ChangeThe change in request.rb:100 from bundle_timestamp to pool.rsc_bundle_hash seems unrelated to the race condition fix. If this is a separate bug fix, it should be mentioned in the PR description. Security & PerformanceNo security concerns identified. Performance is improved through lazy initialization and singleton pattern. Action Items Before Merge
Overall AssessmentApproval Status: Approved with minor changes required This is a well-thought-out fix for a real race condition. The implementation is clean and the documentation is excellent. The main concern is the test coverage gap - please update the RSpec test before merging. Quality: 8/10 Great work on identifying and fixing this subtle race condition! |
This reverts commit 70cc5ef.
This reverts commit 7c3e397.
Previously, `onResponseStart(response)` was called with `void` (fire-and-forget), allowing `handleIncrementalRenderStream` to complete before the HTTP response was fully sent. This caused empty responses when all NDJSON data arrived at once (e.g., when no `sleep` statements were present in async props blocks). The fix captures the promise and awaits it at the end of the function, ensuring the response stream is fully sent before the function returns. Co-Authored-By: Claude Opus 4.5 <[email protected]>
AbanoubGhadban
changed the title
Code Review: Fix async props race conditionSummaryThis PR successfully addresses a critical race condition (issue #2270) where async props fail to deliver when emit.call is invoked without artificial delays. The solution implements lazy initialization via getOrCreateAsyncPropsManager. ✅ What's Good1. Excellent Design Pattern
2. Comprehensive Implementation
3. Clean API Changes 4. Good Documentation 5. Debug Logs Removed 🔍 Areas for Improvement1. Missing Test for Race Condition (Medium Priority) Recommendation: Add test simulating update chunks arriving first. 2. Type Safety Enhancement (Low Priority) 3. Documentation Enhancement (Low Priority) 🔬 Code Quality✅ Thread Safety: sharedExecutionContext scoped to single HTTP request 📋 Pre-Merge Checklist (CLAUDE.md)
🎯 Final VerdictAPPROVE with minor suggestions Solid fix for a real race condition. Core implementation is sound. Suggestions are for enhancing test coverage and documentation, not fixing critical issues. Great work! 🚀 |
Code Review: Fix async props race conditionsSummaryThis PR addresses two critical race conditions in async props delivery for RSC (React Server Components) streaming. The fixes are well-thought-out and properly address the root causes described in issue #2270. ✅ Strengths1. Excellent Problem Analysis
2. Clean API DesignThe
3. Consistent ApplicationThe fix is applied consistently across all access points:
4. Good Documentation
|
Code Review - PR #2297: Fix async props race conditionsSummaryThis PR effectively addresses two critical race conditions in the async props streaming implementation. The fixes are well-designed and properly documented. Overall, this is high-quality work with excellent attention to detail. ✅ Strengths
🔍 Code Quality ObservationsTypeScript/JavaScript Changespackages/react-on-rails-pro-node-renderer/src/worker/handleIncrementalRenderStream.ts:40,100,134
packages/react-on-rails-pro/src/AsyncPropsManager.ts:152-164
packages/react-on-rails-pro/src/ReactOnRailsRSC.ts:146
Ruby Changesreact_on_rails_pro/lib/react_on_rails_pro/async_props_emitter.rb:65,74
react_on_rails_pro/lib/react_on_rails_pro/request.rb:100
react_on_rails_pro/lib/react_on_rails_pro/server_rendering_js_code.rb:77-80
🐛 Potential IssuesMinor: Null Safety in handleIncrementalRenderStream.tsLocation: await onResponseStartPromise;Issue: If the stream ends without processing any chunks (e.g., empty input), Suggestion: Add a null check for clarity: if (onResponseStartPromise) {
await onResponseStartPromise;
}Or initialize to let onResponseStartPromise: Promise<void> = Promise.resolve();🔒 Security Considerations
⚡ Performance Considerations
🧪 Test Coveragereact_on_rails_pro/spec/dummy/spec/requests/incremental_rendering_integration_spec.rb
📋 Minor Suggestions
🎯 ConclusionThis is a well-crafted PR that solves legitimate race conditions with clean, maintainable code. The fixes are architecturally sound and properly documented. Recommendation: ✅ Approve with minor suggestions The minor issues mentioned above are not blockers - they're polish items that could be addressed now or in a follow-up PR. The core fixes are solid and ready to merge. Great work on identifying and fixing these subtle timing issues! 🎉 Review conducted following CLAUDE.md guidelines | Generated with Claude Code |
Code Review - PR #2297: Fix async props race conditionsSummaryThis PR addresses two critical race conditions in the async props streaming implementation. The fixes are well-architected and properly handle edge cases where update chunks arrive before or simultaneously with the initial render request. ✅ Strengths1. Excellent Problem Analysis
2. Clean Implementation
3. Type Safety
4. Documentation
🔍 Issues and ConcernsCRITICAL: Potential Race Condition in
|
Code Review: Fix async props race conditionsOverviewThis PR addresses two critical race conditions in the async props streaming implementation. The fixes are well-designed and properly handle timing issues between initial render and update chunks. ✅ Strengths1. Excellent Race Condition AnalysisThe PR clearly identifies and fixes two distinct race conditions:
2. Clean Lazy Initialization PatternThe getOrCreateAsyncPropsManager() function is well-implemented with clear documentation explaining the race condition, proper lazy initialization pattern, and consistent key usage. 3. Proper Promise HandlingThe fix in handleIncrementalRenderStream.ts correctly captures and awaits the response promise, ensuring the HTTP response is fully sent before the function returns. 4. Comprehensive API Updates
5. Improved Error LoggingBetter error context in async_props_emitter.rb with backtrace information 🔍 Code Quality Observations✅ Consistent pattern usage across Ruby and TypeScript sides
|
Add comprehensive tests for the lazy initialization factory function that handles race conditions between initial render and update chunks: - Update chunks arriving BEFORE initial render (setProp then getProp) - Update chunks arriving DURING initial render (interleaved operations) - Update chunks arriving AFTER initial render (getProp then setProp) - Multiple props set before any getProp - endStream called from update chunk context - Manager isolation between different execution contexts These tests verify that getOrCreateAsyncPropsManager returns the same AsyncPropsManager instance regardless of call order, ensuring props set by update chunks are visible to the initial render and vice versa. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Code Review for PR #2297: Fix async props race conditionsSummaryThis PR fixes two critical race conditions in the async props streaming implementation. The fixes are well-thought-out and properly address the root causes. ✅ Strengths
🔍 Code Quality ObservationsJavaScript/TypeScript SidehandleIncrementalRenderStream.ts (lines 40, 100, 134) let onResponseStartPromise: Promise<void> | null = null;
// ...
onResponseStartPromise = Promise.resolve(onResponseStart(response));
// ...
await onResponseStartPromise;✅ Good: Properly captures and awaits the promise if (onResponseStartPromise) {
await onResponseStartPromise;
}AsyncPropsManager.ts (lines 152-164) Ruby Sideasync_props_emitter.rb (lines 37-39) server_rendering_js_code.rb (lines 79) var { props: propsWithAsyncProps } = ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps, sharedExecutionContext);✅ Good: Now passes request.rb (line 99) emitter = ReactOnRailsPro::AsyncPropsEmitter.new(pool.rsc_bundle_hash, request)✅ Simplified: Always uses 🎯 Performance Considerations
🔒 Security Considerations✅ No security issues identified
📋 Test Coverage AssessmentNew Tests Added:
Updated Tests:
Missing Tests: 🐛 Potential Issues
📝 Suggestions
// In handleIncrementalRenderStream.ts, line 133-134
await onRequestEnded();
if (onResponseStartPromise) {
await onResponseStartPromise;
}
✅ Recommended Actions Before Merge
🎉 Overall AssessmentRating: Excellent ⭐⭐⭐⭐⭐ This is a high-quality PR that properly fixes subtle race conditions with:
The issues mentioned above are minor and don't block merging. Great work! 🚀 Generated by Claude Code Review |
Summary
Fixes two race conditions causing async props to not be delivered when
emit.callis invoked without delays.Fix 1: Lazy AsyncPropsManager initialization
AsyncPropsManagerwas initializedgetOrCreateAsyncPropsManager()for lazy initialization on first accessFix 2: Await response stream completion
handleIncrementalRenderStreamreturned before HTTP response was fully sentvoid onResponseStart()to capture and await the promiseFixes #2270
Test plan
stream_react_component_with_async_propswithout sleep delays🤖 Generated with Claude Code