Switch Pro renderer client to async-http#3279
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Code Review: Switch Pro renderer client to async-httpOverviewThis PR replaces the Bugs / Correctness
rescue SocketError, IOError, Errno::ECONNRESET, Errno::EPIPE, Protocol::HTTP::RefusedError => e
raise ConnectionError, e.message
Dead code:
Style / MaintainabilityRedundant exception-chaining idiom in The pattern inside
MissingNo RBS signatures for Other public classes in the gem have Acknowledged trade-offs (no action required)
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| end | ||
| rescue Async::TimeoutError, IO::TimeoutError => e | ||
| raise TimeoutError, e.message | ||
| rescue SocketError, IOError, Errno::ECONNRESET, Errno::EPIPE, Protocol::HTTP::RefusedError => e |
There was a problem hiding this comment.
Errno::ECONNREFUSED is missing here. Protocol::HTTP::RefusedError is a protocol-level concept; when the Node renderer process is simply not running, async-http surfaces a raw Errno::ECONNREFUSED at the socket level. Without catching it, a downed renderer will produce an unrescued Errno::ECONNREFUSED instead of a clean ConnectionError.
| rescue SocketError, IOError, Errno::ECONNRESET, Errno::EPIPE, Protocol::HTTP::RefusedError => e | |
| rescue SocketError, IOError, Errno::ECONNRESET, Errno::EPIPE, | |
| Errno::ECONNREFUSED, Protocol::HTTP::RefusedError => e |
| # This method is considered as an override of response.each_line. | ||
| # It yields the final partial line even when the renderer returns an error status. | ||
| def loop_response_lines(response) | ||
| return enum_for(__method__, response) unless block_given? |
There was a problem hiding this comment.
A few lines below here (line 165): response.instance_variable_set(:@react_on_rails_received_first_chunk, true) is dead code. This ivar was read by the old HTTPX retry_after lambda in create_connection to abort retries once streaming had begun. That callback no longer exists in the new async-http adapter, and nothing in the current codebase reads this ivar. It should be removed.
| rescue ReactOnRailsPro::RendererHttpClient::Error => e | ||
| begin | ||
| raise e | ||
| rescue StandardError | ||
| raise ReactOnRailsPro::Error, "Failed to fetch dev-server asset from #{path}: #{e}" |
There was a problem hiding this comment.
The inner begin; raise e; rescue StandardError; … end wrapper is redundant. You are already inside rescue … => e, so $! is e and Ruby will automatically set cause on any new exception raised from this rescue block. This simplifies to:
| rescue ReactOnRailsPro::RendererHttpClient::Error => e | |
| begin | |
| raise e | |
| rescue StandardError | |
| raise ReactOnRailsPro::Error, "Failed to fetch dev-server asset from #{path}: #{e}" | |
| rescue ReactOnRailsPro::RendererHttpClient::Error => e | |
| raise ReactOnRailsPro::Error, "Failed to fetch dev-server asset from #{path}: #{e}" |
| end | ||
|
|
||
| class Response | ||
| attr_accessor :status |
There was a problem hiding this comment.
status only needs to be writable from execute_request inside the same class. Exposing the writer publicly via attr_accessor means external callers could overwrite the status after the response is received. Consider attr_reader :status and assigning via @status = inside execute_request to keep mutation internal.
| end | ||
| rescue Async::TimeoutError, IO::TimeoutError => e | ||
| raise TimeoutError, e.message | ||
| rescue SocketError, IOError, Errno::ECONNRESET, Errno::EPIPE, Protocol::HTTP::RefusedError => e |
There was a problem hiding this comment.
Missing Errno::ECONNREFUSED — when the node renderer process is not running, the OS raises Errno::ECONNREFUSED. That error is not in this rescue list, so it will bubble up as an unhandled exception instead of a tidy ConnectionError.
Protocol::HTTP::RefusedError is a protocol-level concept (e.g., HTTP/2 RST_STREAM), not the TCP-layer refused-connection errno, so it doesn't cover this case.
| rescue SocketError, IOError, Errno::ECONNRESET, Errno::EPIPE, Protocol::HTTP::RefusedError => e | |
| rescue SocketError, IOError, Errno::ECONNRESET, Errno::EPIPE, Errno::ECONNREFUSED, Protocol::HTTP::RefusedError => e |
| def stream_body(raw_response, yielder) | ||
| raw_response.body&.each { |chunk| yielder.call(chunk) } | ||
| ensure | ||
| raw_response.body&.close if raw_response&.body |
There was a problem hiding this comment.
Double raw_response.body call in the ensure clause. raw_response.body is evaluated twice — once for the nil guard and once for .close. If calling body has side effects (e.g., buffering the body in async-http), this could be surprising. Simplify to a single call using safe navigation:
| raw_response.body&.close if raw_response&.body | |
| raw_response.body&.close |
|
|
||
| def with_client(&block) | ||
| endpoint = endpoint_for(@origin) | ||
| Async::HTTP::Client.open(endpoint, protocol: endpoint.protocol, retries: 1, limit: @pool_size) do |client| |
There was a problem hiding this comment.
Transport-level retries: 1 stacks on top of application-level retries. Async::HTTP::Client will silently retry once at the transport layer on connection reset/closed, independently of ReactOnRailsPro.configuration.renderer_request_retry_limit. A non-streaming render request that hits a transient error could therefore be attempted up to (1 + renderer_request_retry_limit) * 2 times total, which may surprise operators watching logs.
Consider setting retries: 0 here and letting the application-level retry loop in perform_request be the single authority, or document the double-retry explicitly in the configuration docs.
| # @param value [Numeric, nil] A positive number or nil (to use the HTTPX default) | ||
| # @param value [Numeric, nil] A positive number or nil | ||
| # @raise [ReactOnRailsPro::Error] if value is not a positive number or nil | ||
| def renderer_http_keep_alive_timeout=(value) |
There was a problem hiding this comment.
Silent no-op config — consider emitting a deprecation warning. The setter accepts and validates the value but never applies it. Users who have config.renderer_http_keep_alive_timeout = 25 in their initializer will get no feedback that it now does nothing.
A warn on assignment (like the project does for RENDERER_BUNDLE_PATH) would surface the behavior change during app boot:
def renderer_http_keep_alive_timeout=(value)
unless value.nil? || (value.is_a?(Numeric) && value.positive? && value.finite?)
raise ReactOnRailsPro::Error,
"config.renderer_http_keep_alive_timeout must be a finite positive number or nil"
end
warn "[ReactOnRailsPro] config.renderer_http_keep_alive_timeout is deprecated and has no effect " \
"with the async-http adapter (clients are scoped per-request)." unless value.nil?
@renderer_http_keep_alive_timeout = value
end
Code ReviewThis is a well-structured transport swap. The new Bugs / Correctness1. 2. Stale HTTPX-era instance variable in response.instance_variable_set(:@react_on_rails_received_first_chunk, true)This was read by the Behavior Changes Worth Discussing3. Transport-level 4. No connection reuse across requests 5. Minor / Style6. Double raw_response.body&.close if raw_response&.body # body evaluated twiceShould be 7. No RBS type signatures for SummaryThe two actionable bugs before merge are |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| def run_with_timeout(&block) | ||
| if (task = Async::Task.current?) | ||
| task.with_timeout(@read_timeout, &block) | ||
| else | ||
| Sync { |sync_task| sync_task.with_timeout(@read_timeout, &block) } | ||
| end |
There was a problem hiding this comment.
Connect timeout vs read timeout budget
@connect_timeout is passed to endpoint_for as the timeout: option, but the with_timeout wrapper here uses @read_timeout to cap the entire operation (TCP connect + TLS handshake + HTTP/2 connection + read). If a TCP connect is slow, it consumes from the read timeout budget rather than the connect timeout budget.
In practice this means a connect_timeout=5 / read_timeout=30 config doesn't behave as users would expect — the connect phase isn't independently bounded by 5s; it's still bounded by 30s total. Worth a brief note in the deprecation/docs for renderer_http_pool_timeout, especially since the old HTTPX client did apply them independently.
| client.get(path, headers: Protocol::HTTP::Headers[headers]) | ||
| end | ||
|
|
||
| response.__send__(:assign_status, raw_response.status) |
There was a problem hiding this comment.
__send__ to call a private method across object boundary
Calling response.__send__(:assign_status, ...) from inside execute_request crosses the private-method contract. The executor block already creates a tight coupling between RendererHttpClient and Response; making assign_status protected (or using a yielded callback) would express that coupling without bypassing Ruby's visibility rules.
Minimal alternative — make it a protected setter instead:
protected
def assign_status(status)
@status = status
endThen the call site can use response.send(:assign_status, ...) with the same effect, but it's a clear "friend class" pattern rather than __send__.
| body << %(Content-Disposition: form-data; name="#{name}"\r\n) | ||
| body << "\r\n" | ||
| body << value.to_s | ||
| body << "\r\n" | ||
| end | ||
|
|
||
| def append_file_part(body, boundary, name, value) | ||
| body << "--#{boundary}\r\n" | ||
| body << %(Content-Disposition: form-data; name="#{name}"; filename="#{value.fetch(:filename)}"\r\n) |
There was a problem hiding this comment.
Content-Disposition header values are interpolated without escaping
name (line 160) and filename (line 168) are interpolated directly into the Content-Disposition header. A double-quote " or embedded \r\n in either value would corrupt the MIME header structure — potentially allowing header injection.
Since these fields come from internal gem code (form keys like "bundle_server_bundle.js", "password", etc.) the real-world risk is low. But if filename ever incorporates a user-supplied or file-system-derived path component, the exposure grows. A defensive guard:
def sanitize_header_param(value)
value.to_s.gsub(/"/, '\\"').gsub(/[\r\n]/, "")
endand usage like name="#{sanitize_header_param(name)}" would eliminate the class of issue entirely.
| def with_client(&block) | ||
| endpoint = endpoint_for(@origin) | ||
| Async::HTTP::Client.open(endpoint, protocol: endpoint.protocol, retries: 0, limit: @pool_size) do |client| | ||
| # rubocop:disable Performance/RedundantBlockCall | ||
| block.call(client) | ||
| # rubocop:enable Performance/RedundantBlockCall | ||
| end |
There was a problem hiding this comment.
pool_size / limit: semantic shift
In the HTTPX world renderer_http_pool_size controlled the number of persistent connections kept in a pool across requests. Here, limit: is passed to Async::HTTP::Client.open, which creates a per-request client whose pool only lives for the duration of a single post/get call. For H2C (forced for the renderer), limit: effectively caps concurrent streams on that single connection, not a long-lived connection pool.
The PR notes say keep_alive_timeout is silently no-op'ed with a deprecation warning. pool_size has a similarly changed meaning and might be worth a similar deprecation note (or at least a config-level comment), so users don't spend time tuning a value that no longer controls what the docs said it did.
| def response_has_error_status?(response) | ||
| return true if response.is_a?(HTTPX::ErrorResponse) | ||
| return response.error? if response.respond_to?(:error?) | ||
|
|
||
| response.status >= 400 | ||
| rescue NoMethodError | ||
| # HTTPX::StreamResponse can fail to delegate #status for non-streaming errors. | ||
| true | ||
| end |
There was a problem hiding this comment.
rescue NoMethodError is now a dead branch
RendererHttpClient::Response always defines error?, so response.respond_to?(:error?) will always be true and we'll always return early. The rescue NoMethodError fallback (and the response.status >= 400 line) can no longer trigger for the current adapter.
This is fine to keep as defensive dead code, but removing the comment that described the HTTPX-specific reason would be a good idea — the original comment was already deleted in this PR, so the only thing left is the rescue clause with no explanation. A brief comment like # guard for non-Response objects returned by future adapters would clarify intent.
Code Review — PR #3279: Switch Pro renderer client to async-httpOverviewA well-scoped transport replacement. The new Correctness
if request_time > warn_timeoutIf
Good defensive addition — prevents a Streaming retry protection removed — intentional and correct The old HTTPX client used an Design / Architecture
Content-Disposition escaping (see inline) Test QualityMuch improved. Removing the custom HTTPX mock plugin (~154 lines) and replacing it with a clean Breaking Changes
SummaryThe core logic is sound and the test coverage is solid. The five inline comments are the main items to evaluate; the connect/read timeout conflation and |
| end | ||
|
|
||
| def build_multipart_body(form, boundary: SecureRandom.hex(24)) | ||
| body = +"" |
There was a problem hiding this comment.
Potential Encoding::CompatibilityError with binary file bodies
body = +"" creates a mutable UTF-8 string. Pathname#binread (line 192) returns ASCII-8BIT. Concatenating them with << raises Encoding::CompatibilityError if the file content contains bytes outside the ASCII range — which can happen with minified JS containing embedded binary assets or certain Unicode characters beyond the BMP.
| body = +"" | |
| body = +"".force_encoding(Encoding::BINARY) |
And in append_scalar_part / append_file_part, ensure the boundary/header strings are ASCII-compatible (they already are since boundary is hex and headers are ASCII), so forcing the body to BINARY here is safe and lets binary file content concatenate without encoding issues.
| filename = sanitize_header_param(value.fetch(:filename)) | ||
| body << "--#{boundary}\r\n" | ||
| body << %(Content-Disposition: form-data; name="#{name}"; filename="#{filename}"\r\n) | ||
| body << "Content-Type: #{value.fetch(:content_type)}\r\n" |
There was a problem hiding this comment.
content_type not sanitized against CRLF injection
name and filename are sanitized via sanitize_header_param, but value.fetch(:content_type) is inserted into the header line directly. A content_type value containing \r\n would inject an extra header field. Since callers are internal, the practical risk is low, but it's inconsistent with the sanitization applied to the other parameters.
| body << "Content-Type: #{value.fetch(:content_type)}\r\n" | |
| body << "Content-Type: #{sanitize_header_param(value.fetch(:content_type))}\r\n" |
| end | ||
| end | ||
|
|
||
| def close |
There was a problem hiding this comment.
No-op close breaks the expectation that Request#reset_connection cleans up resources
Request holds a @connection class-level instance variable and calls @connection.close in reset_connection. With HTTPX this closed the persistent socket pool. With a no-op close, calling reset_connection silently does nothing at the transport level — which is correct for this per-request design, but a future developer who doesn't read both files will be confused.
Consider removing close entirely and letting Request duck-type against respond_to?(:close), or raising NotImplementedError with a clear explanation. At minimum the comment here should say the method must stay for Request compatibility, not just explain why it's a no-op.
| raise ConnectionError, e.message | ||
| end | ||
|
|
||
| def run_with_timeout(&block) |
There was a problem hiding this comment.
@read_timeout bounds the entire operation, including TCP connect
@connect_timeout is passed as the endpoint-level TCP connect timeout (line 282), but @read_timeout (which is ssr_timeout) wraps everything: connect, TLS handshake, request write, and response body streaming. That's the right behavior — SSR rendering must finish within ssr_timeout regardless of where time is spent — but the parameter name read_timeout is misleading here.
A comment like:
# @read_timeout is the ssr_timeout: it bounds the total wall-clock time
# for this request including TCP connect, so a slow connect still counts
# against the SSR deadline.would prevent future confusion with the separate @connect_timeout.
| Async::HTTP::Endpoint.parse(origin, **options) | ||
| end | ||
|
|
||
| def stream_body(raw_response, yielder) |
There was a problem hiding this comment.
raw_response.body accessed twice in stream_body
raw_response.body is called on line 289 (to iterate) and again on line 291 (to close). For most async-http response bodies this is fine, but if the body object is a one-shot object (e.g., a Protocol::HTTP2::Body::Reader) the second call to raw_response.body may return nil after the first each has consumed and released it — making the close a no-op rather than an explicit drain.
It's safer to capture the body once:
| def stream_body(raw_response, yielder) | |
| def stream_body(raw_response, yielder) | |
| body = raw_response&.body | |
| body&.each { |chunk| yielder.call(chunk) } | |
| ensure | |
| body&.close | |
| end |
|
|
||
| it "does not expose a public chunk writer" do | ||
| response = described_class.new(status: 200) | ||
|
|
There was a problem hiding this comment.
SocketError and IOError are in CONNECTION_ERRORS but not covered by this parametrized list. Both are plausible in real-world DNS/socket failure scenarios. Consider adding them:
| [ | |
| SocketError, | |
| IOError, | |
| Errno::ECONNREFUSED, | |
| Errno::ECONNRESET, | |
| Errno::EHOSTUNREACH, | |
| Errno::ENETUNREACH, | |
| Errno::EPIPE, | |
| Errno::ETIMEDOUT, | |
| Protocol::HTTP::RefusedError, | |
| [Protocol::HTTP2::StreamError, "stream reset"] | |
| ].each do |error_class, *args| |
|
|
||
| raise Error, "io-endpoint #{IO::Endpoint::VERSION} is unsupported; async-http renderer client " \ | ||
| "requires #{IO_ENDPOINT_REQUIREMENT} because ConnectTimeoutWrapper relies on " \ | ||
| "IO::Endpoint::Wrapper internals." |
There was a problem hiding this comment.
The error message could be more actionable for gem users. When this fires, they'll see a bare load error and need to figure out what to do. Suggesting a fix in the message costs one line:
| "IO::Endpoint::Wrapper internals." | |
| raise Error, "io-endpoint #{IO::Endpoint::VERSION} is unsupported; async-http renderer client " \ | |
| "requires #{IO_ENDPOINT_REQUIREMENT} because ConnectTimeoutWrapper relies on " \ | |
| "IO::Endpoint::Wrapper internals. " \ | |
| "Pin `gem 'io-endpoint', '#{IO_ENDPOINT_REQUIREMENT}''` in your Gemfile or run `bundle update io-endpoint`." |
| # The mock pre-sets status above; pass the request so specs can assert on the captured payload. | ||
| block.call(->(value) { yielder.call(value) }, request) | ||
| end | ||
| end |
There was a problem hiding this comment.
Mock pre-sets streaming status unlike real lazy behavior
Low Severity
The MockClient#build_response for streaming requests creates a Response with status: pre-set, while the real RendererHttpClient#post(stream: true) returns a Response with status: nil until the executor runs. This means perform_request's STATUS_INCOMPATIBLE guard (response.status && response.status == STATUS_INCOMPATIBLE) fires in tests but is always skipped in production for streaming, exercising a different code path than what runs live.
Reviewed by Cursor Bugbot for commit 76f1a1b. Configure here.
PR 3279 — Switch Pro renderer client to
|
| end | ||
|
|
||
| def connect(remote_address, **options) | ||
| socket = super(remote_address, **options.merge(connect_timeout ? { timeout: connect_timeout } : {})) |
There was a problem hiding this comment.
Minor nit: the ternary merge can be simplified to { timeout: connect_timeout }.compact, which avoids the conditional entirely and reads more clearly when connect_timeout is nil:
| socket = super(remote_address, **options.merge(connect_timeout ? { timeout: connect_timeout } : {})) | |
| socket = super(remote_address, **options.merge({ timeout: connect_timeout }.compact)) |
|
|
||
| def error? | ||
| # nil status means a lazy streaming executor has not run yet, so it is not-yet-an-error. | ||
| !status.nil? && status >= 400 |
There was a problem hiding this comment.
error? returning false for a nil status is intentional (lazy executor hasn't run yet), but it makes error? a three-state concept in practice: false can mean "definitely not an error" or "status unknown". A caller who reads response.error? before triggering the executor will silently get false regardless of the eventual outcome.
If there is any path where external code might call error? before each/body has been driven, consider returning nil (or raising) for the unresolved case instead so callers can't mistake "not yet known" for "success". The current design is safe in the existing call chain, but worth documenting explicitly here so future consumers of Response understand the invariant.
| end | ||
| end | ||
|
|
||
| validate_io_endpoint_version! |
There was a problem hiding this comment.
This class-body call runs at require time, which is the right place for a dependency version guard. One thing to note: the error message tells users to "run bundle update io-endpoint", but if async-http is already pinned to a version whose own declared io-endpoint constraint conflicts with ~> 0.17.0, running bundle update will fail with a resolver conflict rather than silently rolling io-endpoint back. The message could be clearer that a Gemfile pin like gem "io-endpoint", "~> 0.17.0" may be required, not just a bundle update.
| def pool_limit | ||
| return @pool_size unless @pool_size.nil? | ||
|
|
||
| if defined?(ReactOnRailsPro::Configuration::DEFAULT_RENDERER_HTTP_POOL_SIZE) | ||
| ReactOnRailsPro::Configuration::DEFAULT_RENDERER_HTTP_POOL_SIZE | ||
| else | ||
| 10 | ||
| end | ||
| end |
There was a problem hiding this comment.
The defined? guard makes RendererHttpClient loadable in isolation (e.g., in specs that require only this file), which is a valid reason to have it. However, the hardcoded 10 fallback is dead code in any realistic production or test scenario — Configuration is always loaded when the gem is in use. If you want to keep the guard for standalone-require safety, at least document why the magic 10 exists:
def pool_limit
return @pool_size unless @pool_size.nil?
# Fall back to the configuration default. The defined? guard lets this file be
# required in isolation (e.g., unit specs) without pulling in the full gem.
if defined?(ReactOnRailsPro::Configuration::DEFAULT_RENDERER_HTTP_POOL_SIZE)
ReactOnRailsPro::Configuration::DEFAULT_RENDERER_HTTP_POOL_SIZE
else
10 # matches Configuration::DEFAULT_RENDERER_HTTP_POOL_SIZE
end
endAlternatively, move the constant out of Configuration and into a shared module so RendererHttpClient can reference it without a guard.
|
|
||
| while (idx = line.index("\n")) | ||
| yield line.byteslice(0..idx - 1) | ||
| yield line.byteslice(0, idx) |
There was a problem hiding this comment.
This is the fix for the data-duplication bug. Worth calling out explicitly for future readers: the old form was byteslice(0..idx - 1), which is an inclusive range. When idx == 0 (the chunk starts with \n), 0..-1 returns the entire line buffer, causing the buffered content to be yielded a second time. byteslice(0, idx) with a length argument returns "" when idx == 0, which is correct — an empty line is yielded, and the buffer advances past the newline.
| def run_with_timeout(&block) | ||
| # @read_timeout carries ssr_timeout and bounds the whole renderer request, | ||
| # including TCP connect, request write, and response body streaming. | ||
| # Async treats nil as no timeout; configuration keeps that legacy escape hatch explicit. | ||
| if (task = Async::Task.current?) | ||
| task.with_timeout(@read_timeout, &block) | ||
| else | ||
| # Rails calls this from a synchronous request thread; Sync blocks that thread | ||
| # while async-http drives the renderer exchange inside a temporary reactor. | ||
| # If an async server reaches this fallback from inside an existing reactor without | ||
| # Async::Task.current?, Sync may create a nested reactor and deadlock; rework this | ||
| # path before supporting that setup. | ||
| Sync { |sync_task| sync_task.with_timeout(@read_timeout, &block) } | ||
| end | ||
| end |
There was a problem hiding this comment.
The comment is clear, but the failure mode (deadlock) is silent and extremely hard to diagnose in a production Falcon deployment. Consider making it loud instead:
def run_with_timeout(&block)
if (task = Async::Task.current?)
task.with_timeout(@read_timeout, &block)
elsif defined?(Async::Reactor) && Async::Reactor.current
# Called from inside a Falcon/async-Rails reactor without a task context.
# Creating a nested reactor here can deadlock. Raise immediately rather than hang.
raise ReactOnRailsPro::Error,
"[ReactOnRailsPro] RendererHttpClient called from inside an Async reactor " \
"without Async::Task.current?. Falcon/async-Rails deployments are not yet " \
"supported. See the upgrade guide."
else
Sync { |sync_task| sync_task.with_timeout(@read_timeout, &block) }
end
endA clear error on first use beats a silent hang at peak load.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
Bugbot Autofix is ON, but it could not run because on-demand usage is turned off. To enable Bugbot Autofix, turn on on-demand usage and set a spend limit in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 989c80d. Configure here.
| connection_pool | ||
| execjs (~> 2.9) | ||
| http-2 (>= 1.1.1) | ||
| httpx (~> 1.5) |
There was a problem hiding this comment.
Lock file missing direct io-endpoint gemspec dependency listing
Low Severity
The execjs-compatible-dummy/Gemfile.lock lists react_on_rails_pro's PATH dependencies but omits io-endpoint (~> 0.17.0), unlike the main Gemfile.lock and spec/dummy/Gemfile.lock which both include it. The gemspec declares io-endpoint as a direct runtime dependency. The gem is resolved transitively at the correct version (0.17.2) through async-http, but the missing constraint means the lock file doesn't reflect the gemspec's pinning intent.
Reviewed by Cursor Bugbot for commit 989c80d. Configure here.
Code ReviewOverviewThis PR replaces the Pro renderer transport layer (HTTPX to async-http) cleanly. The breaking change is well-documented throughout CHANGELOG, upgrade guide, config docs, and README. The test suite was rebuilt from scratch without HTTPX mock internals, which is the right move. The new Critical / Pre-Merge ConcernPer-request client creation removes persistent connection reuse. Every renderer call (render, streaming render, asset upload) now opens a fresh TCP socket and completes an HTTP/2 handshake. On loopback this costs ~1-2 ms (per the benchmark in the PR), but in any topology where Rails and the renderer run on different hosts each request adds a full TCP + HTTP/2 handshake round-trip. At 1000 req/s that is non-trivial overhead. The PR documents this in the changelog and issue #3283, but the PR description should state clearly whether this is a known regression or expected to be neutral for the benchmark target environment. Code Issues1. The subclass calls 2. The 3. If an async server (Falcon, etc.) calls into the renderer from inside a task that does not expose 4.
5. In Minor / Nits
SummaryThe approach is solid and well-tested. The two items I would want addressed before merge: (1) a clear statement about whether per-request connection overhead is acceptable for the target topology or a firmer plan/timeline for #3283, and (2) the |
| end | ||
| end | ||
|
|
||
| validate_io_endpoint_version! |
There was a problem hiding this comment.
This class-level call to validate_io_endpoint_version! runs at require time — not lazily. That is intentional (fail-fast), but it means any require of this file (including in unit tests that mock the HTTP client) will raise if io-endpoint is outside the pinned range. Tests that stub RendererHttpClient entirely can still be broken at load time if the constant changes. Worth a one-line comment here explaining the fail-fast intent so the call doesn't look like an accidental top-level invocation.
| Async::HTTP::Endpoint.parse(origin, **options) | ||
| end | ||
|
|
||
| def pool_limit |
There was a problem hiding this comment.
The defined? guard is defensive but unnecessary: RendererHttpClient is always loaded alongside Configuration (same gem, same load path), so DEFAULT_RENDERER_HTTP_POOL_SIZE is always available. The fallback 10 is dead code that will silently diverge if the constant's value changes. Suggest simplifying to:
| def pool_limit | |
| def pool_limit | |
| return @pool_size unless @pool_size.nil? | |
| ReactOnRailsPro::Configuration::DEFAULT_RENDERER_HTTP_POOL_SIZE |
| if (task = Async::Task.current?) | ||
| task.with_timeout(@read_timeout, &block) | ||
| else | ||
| # Rails calls this from a synchronous request thread; Sync blocks that thread | ||
| # while async-http drives the renderer exchange inside a temporary reactor. | ||
| # If an async server reaches this fallback from inside an existing reactor without | ||
| # Async::Task.current?, Sync may create a nested reactor and deadlock; rework this | ||
| # path before supporting that setup. | ||
| Sync { |sync_task| sync_task.with_timeout(@read_timeout, &block) } | ||
| end |
There was a problem hiding this comment.
The comment correctly describes the nested-reactor deadlock risk, but the fallback silently takes the Sync path and deadlocks rather than raising an actionable error. For async Rails servers (Falcon, etc.) that inadvertently reach this branch, a deadlock will surface as a hung request with no stack trace pointing here.
Consider making the unsafe path explicit:
def run_with_timeout(&block)
if (task = Async::Task.current?)
task.with_timeout(@read_timeout, &block)
else
# Sync creates a temporary reactor. If called from inside an existing reactor
# without Async::Task.current? (e.g. a Falcon middleware shim), this may
# deadlock. Raise immediately so the caller gets an actionable error.
raise NotImplementedError,
"[ReactOnRailsPro] RendererHttpClient cannot be called from inside an " \
"existing Async reactor without a current task. Use a standard synchronous " \
"Rails server (Puma) or wait for async-reactor support in issue #3283." \
if inside_async_reactor?
Sync { |sync_task| sync_task.with_timeout(@read_timeout, &block) }
end
endEven a TODO comment explicitly saying "raise here once detection is reliable" would help future maintainers.
|
|
||
| while (idx = line.index("\n")) | ||
| yield line.byteslice(0..idx - 1) | ||
| yield line.byteslice(0, idx) |
There was a problem hiding this comment.
The byteslice(0, idx) fix is correct: when idx == 0 (chunk starts with \n), the old 0..idx-1 range expanded to 0..-1 (the entire buffer), so the buffered content would be duplicated. The new form yields an empty string "" instead, which is then filtered upstream by chunk.strip in process_response_chunks. The fix and the upstream filter work together correctly — worth a cross-reference comment here so the empty-yield is not later "optimized" away by a reader who doesn't know about the strip filter above.
| end | ||
|
|
||
| if response_has_error_status?(stream_response) | ||
| error_body << chunk |
There was a problem hiding this comment.
Error chunks are concatenated without a separator. Since loop_response_lines splits on \n and strips the delimiter, a renderer error body with multiple lines like:
Error: VM context failed
at render (renderer.js:42)
becomes "Error: VM context failed at render (renderer.js:42)" — unreadable. Consider error_body << chunk << "\n" so the error body preserves line boundaries when surfaced in handle_http_error.
Code Review: Switch Pro renderer client to async-httpOverall assessment: This is a significant, well-documented transport layer replacement with strong test coverage and thorough validation. The implementation is generally clean and the mock infrastructure is a genuine improvement. A few areas deserve attention before merge. Summary of Changes
Issues1. Request-scoped clients = no connection reuse (documented but high risk)Every renderer request opens a new TCP + HTTP/2 connection. The PR acknowledges this in the changelog and docs, and tracks persistent connection reuse in Issue 3283. However, the benchmark numbers (1461 req/s, avg 6.73 ms) are loopback benchmarks—in production topologies where Rails and the renderer run on different hosts, each request pays a full TCP handshake + HTTP/2 SETTINGS frame exchange. For high-throughput or latency-sensitive deployments, this regression could be significant. Worth flagging prominently in the PR description and considering a fast-follow milestone target for Issue 3283. 2. Nested reactor deadlock risk (Falcon / async-rails)The 3.
|
| # If an async server reaches this fallback from inside an existing reactor without | ||
| # Async::Task.current?, Sync may create a nested reactor and deadlock; rework this | ||
| # path before supporting that setup. | ||
| Sync { |sync_task| sync_task.with_timeout(@read_timeout, &block) } |
There was a problem hiding this comment.
This fallback silently creates a new Async reactor in a synchronous Rails request thread, which works fine for Puma but will deadlock on Falcon/async-rails (as the comment notes). Since those deployments are explicitly unsupported, consider raising rather than silently proceeding:
| Sync { |sync_task| sync_task.with_timeout(@read_timeout, &block) } | |
| raise ReactOnRailsPro::Error, | |
| "[ReactOnRailsPro] renderer requests from inside an existing Async reactor are not supported. " \ | |
| "Async Rails servers (Falcon, etc.) must not call the Node renderer from a request path that " \ | |
| "already has an Async::Task. See Issue 3283." |
A loud error is safer than a mysterious deadlock for users on async-compatible servers.
| Async::HTTP::Endpoint.parse(origin, **options) | ||
| end | ||
|
|
||
| def pool_limit |
There was a problem hiding this comment.
The defined? check is unnecessary — DEFAULT_RENDERER_HTTP_POOL_SIZE is a constant defined in the same gem and will always be present. The fallback 10 is also identical to the constant's value, which is confusing because it implies the constant might not exist.
| def pool_limit | |
| def pool_limit | |
| @pool_size.nil? ? ReactOnRailsPro::Configuration::DEFAULT_RENDERER_HTTP_POOL_SIZE : @pool_size | |
| end |
| Protocol::HTTP2::StreamError | ||
| ].freeze | ||
|
|
||
| class ConnectTimeoutWrapper < IO::Endpoint::Wrapper |
There was a problem hiding this comment.
This class directly subclasses IO::Endpoint::Wrapper and relies on two undocumented internal contracts: (1) Wrapper#initialize accepts zero arguments, and (2) Wrapper#connect calls socket_connect internally (so the super call in connect can intercept the timeout). The gemspec pins io-endpoint ~> 0.17.0 and there's a version-check test, which is the right mitigation.
One concern: clear_timeout is called on the socket before the yield socket block runs. This means any code in the block receives a socket with timeout = nil regardless of what happens inside the block. This is intentional (the connect timeout should not persist into I/O reads), but it's worth an explicit comment so future readers don't try to "fix" it:
| class ConnectTimeoutWrapper < IO::Endpoint::Wrapper | |
| def connect(remote_address, **options) | |
| socket = super(remote_address, **options.merge(connect_timeout ? { timeout: connect_timeout } : {})) | |
| # Clear the per-socket timeout immediately after connect so read/write timeouts | |
| # are driven by the Async task timeout (ssr_timeout), not the connect timeout. | |
| clear_timeout(socket) | |
| return socket unless block_given? | |
| begin | |
| yield socket | |
| ensure | |
| socket.close | |
| end | |
| end |
|
|
||
| while (idx = line.index("\n")) | ||
| yield line.byteslice(0..idx - 1) | ||
| yield line.byteslice(0, idx) |
There was a problem hiding this comment.
This is the core bug fix: when idx = 0 (chunk starts with \n), the old byteslice(0..idx - 1) evaluated as byteslice(0..-1) which returned the entire buffered line — duplicating it. byteslice(0, idx) correctly returns "" when idx = 0. The fix is correct and the new test in stream_request_spec.rb ("does not duplicate a line when a chunk starts with a newline") covers it well.
| end | ||
| end | ||
|
|
||
| validate_io_endpoint_version! |
There was a problem hiding this comment.
Calling validate_io_endpoint_version! at class body load time (outside any method) means this runs the moment renderer_http_client.rb is required. This is a good pattern — it fails fast at boot/require rather than at first renderer request. Just worth noting this is intentional: the validate call is at class scope, not inside initialize.
|
View the full report in the job summary. |
3 participants
Summary
httpx/http-2with a smallasync-httpadapter.RendererHttpClient::Response.async-httpstack.Notes for Review
Async::HTTP::Protocol::HTTP2; dev-server asset GETs do not force HTTP/2 because those URLs can point at arbitrary HTTP servers.async-httpclients to individual requests so clients never cross Async reactors. That meansrenderer_http_keep_alive_timeoutis retained for configuration compatibility but is no longer applied by this adapter; assigning a non-nil value now warns.content-length;async-http/protocol-http2sets it, and duplicating it caused Fastify HTTP/2 stream resets during testing.Validation
bundle exec rspec spec/react_on_rails_pro/renderer_http_client_spec.rb spec/react_on_rails_pro/request_spec.rb spec/react_on_rails_pro/stream_spec.rb spec/react_on_rails_pro/server_rendering_pool/node_rendering_pool_spec.rb- 44 examples, 0 failures.Review follow-up:
bundle exec rspec spec/react_on_rails_pro/stream_request_spec.rb spec/react_on_rails_pro/renderer_http_client_spec.rb spec/react_on_rails_pro/configuration_spec.rbinreact_on_rails_pro- 63 examples, 0 failures.Review follow-up:
bundle exec rspec spec/react_on_rails_proinreact_on_rails_pro- 315 examples, 0 failures.Pro dummy manual:
bundle exec rake react_on_rails:generate_packsinreact_on_rails_pro/spec/dummy.Pro dummy manual:
pnpm run build:testinreact_on_rails_pro/spec/dummy- passed with existing DefinePlugin/import-meta warnings.Pro dummy manual:
bundle exec rspec spec/helpers/react_on_rails_pro_helper_spec.rb spec/requests/upload_asset_spec.rbinreact_on_rails_pro/spec/dummywith the node renderer running - 57 examples, 0 failures.OSS dummy manual:
mise exec ruby@3.2.9 -- bundle exec rake react_on_rails:generate_packsinreact_on_rails/spec/dummy.OSS dummy manual:
mise exec ruby@3.2.9 -- pnpm run build:testinreact_on_rails/spec/dummy- passed with existing DefinePlugin warnings.OSS dummy manual:
mise exec ruby@3.2.9 -- bundle exec rspec spec/helpers/react_on_rails_helper_spec.rb spec/requests/server_render_check_spec.rb spec/packs_generator_spec.rbinreact_on_rails/spec/dummy- 240 examples, 0 failures when run outside the sandbox for Capybara localhost binding.pnpm run build.pnpm run lint.pnpm start format.listDifferent.bundle exec rake rbs:validate.git diff --check.Touched-file
bundle exec rubocop --cache-root /private/tmp/rubocop_cache_async_http_review --no-server --cache false ...- 7 files inspected, no offenses detected.Pre-commit hook on commit
91531221d- trailing-newlines and changed-file RuboCop passed.Pre-push hook after commit
91531221d- branch RuboCop on 13 changed Ruby files and online markdown-link checks passed.Heartbeat follow-up:
bundle exec rspec spec/react_on_rails_pro/renderer_http_client_spec.rb spec/react_on_rails_pro/request_spec.rb spec/react_on_rails_pro/stream_request_spec.rb spec/react_on_rails_pro/configuration_spec.rbinreact_on_rails_pro- 91 examples, 0 failures.Heartbeat follow-up:
bundle exec rspec spec/react_on_rails_proinreact_on_rails_pro- 322 examples, 0 failures.Heartbeat follow-up: focused
bundle exec rubocop --cache-root /private/tmp/rubocop_cache_async_http_heartbeat --no-server --cache false ...- 8 files inspected, no offenses detected.Heartbeat follow-up:
git diff --check.Pre-commit hook on commit
c22faaaee- trailing-newlines, autofix, and changed-file RuboCop passed.Pre-push hook after commit
c22faaaee- branch RuboCop on 13 changed Ruby files and online markdown-link checks passed.Review follow-up commit
4425e64f0:bundle exec rspec spec/react_on_rails_pro/renderer_http_client_spec.rbinreact_on_rails_pro- 17 examples, 0 failures.Review follow-up commit
4425e64f0:bundle exec rspec spec/react_on_rails_pro/renderer_http_client_spec.rb spec/react_on_rails_pro/request_spec.rb spec/react_on_rails_pro/stream_request_spec.rb spec/react_on_rails_pro/configuration_spec.rbinreact_on_rails_pro- 94 examples, 0 failures.Review follow-up commit
4425e64f0:bundle exec rspec spec/react_on_rails_proinreact_on_rails_pro- 325 examples, 0 failures.Review follow-up commit
4425e64f0: focusedbundle exec rubocop --cache-root /private/tmp/rubocop_cache_async_http_post --no-server --cache false ...- 2 files inspected, no offenses detected.Review follow-up commit
4425e64f0:git diff --check.Pre-commit hook on commit
4425e64f0- trailing-newlines, autofix, and changed-file RuboCop passed.Pre-push hook after commit
4425e64f0- branch RuboCop on 13 changed Ruby files and online markdown-link checks passed.Review follow-up commit
960e91b14:bundle exec rspec spec/react_on_rails_pro/renderer_http_client_spec.rb spec/react_on_rails_pro/stream_request_spec.rbinreact_on_rails_pro- 21 examples, 0 failures.Review follow-up commit
960e91b14:bundle exec rspec spec/react_on_rails_pro/renderer_http_client_spec.rb spec/react_on_rails_pro/request_spec.rb spec/react_on_rails_pro/stream_request_spec.rb spec/react_on_rails_pro/configuration_spec.rbinreact_on_rails_pro- 96 examples, 0 failures.Review follow-up commit
960e91b14:bundle exec rspec spec/react_on_rails_proinreact_on_rails_pro- 327 examples, 0 failures.Review follow-up commit
960e91b14: focusedbundle exec rubocop --cache-root /private/tmp/rubocop_cache_async_http_comments --no-server --cache false ...- 4 files inspected, no offenses detected.Review follow-up commit
960e91b14:pnpm start format.listDifferent.Review follow-up commit
960e91b14:git diff --check.Pre-commit hook on commit
960e91b14- trailing-newlines, markdown-links, Prettier, and changed-file RuboCop passed.Pre-push hook after commit
960e91b14- branch RuboCop on 14 changed Ruby files and online markdown-link checks passed.Review follow-up commit
c03127d7f:bundle exec rake rbs:validate- passed.Pre-commit hook on commit
c03127d7f- trailing-newlines passed.Pre-push hook after commit
c03127d7f- branch RuboCop on 14 changed Ruby files and online markdown-link checks passed.Follow-up commit
1dd09644e:bundle exec rspec spec/react_on_rails_proinreact_on_rails_pro- 329 examples, 0 failures.Follow-up commit
1dd09644e: Pro dummy node renderer manual smoke,bundle exec rspec spec/helpers/react_on_rails_pro_helper_spec.rb spec/requests/upload_asset_spec.rbinreact_on_rails_pro/spec/dummy- 57 examples, 0 failures.Follow-up commit
1dd09644e: repeatbundle exec rspec spec/requests/upload_asset_spec.rbtwice against the same live node renderer - 3 examples, 0 failures each run; verifies renderer cache cleanup for uploaded assets.Follow-up commit
1dd09644e: OSS dummy manual smoke,bundle exec rspec spec/helpers/react_on_rails_helper_spec.rb spec/requests/server_render_check_spec.rbinreact_on_rails/spec/dummy- 118 examples, 0 failures when run outside the sandbox for Capybara localhost binding.Follow-up commit
1dd09644e:bundle exec rake rbs:validate,pnpm start format.listDifferent,pnpm run build, touched-file RuboCop,git diff --check, pre-commit, and pre-push hooks passed.Local transport smoke benchmark against the Pro dummy node renderer
/asset-exists: sequential 100 requests avg 1.68 ms, p50 1.35 ms, p95 4.87 ms, max 5.07 ms; concurrent 200 requests across 10 threads, 1461 req/s, avg 6.73 ms, p95 12.74 ms.Full
bundle exec rubocop --cache-root /private/tmp/rubocop_cache_async_http_review --no-server --cache falsestill fails on the existing repo baseline: 550 offenses under generated/support files such asreact_on_rails_pro/spec/dummy/bin/*,react_on_rails_pro/spec/dummy/db/schema.rb, andscript/convert.Note
High Risk
Replaces the Pro Rails-to-node-renderer transport layer and changes connection/pooling/timeout semantics for SSR and streaming, plus bumps the minimum Ruby version to 3.3—both can impact production rendering behavior and performance.
Overview
React on Rails Pro now uses
async-httpinstead ofhttpxfor all Rails→Node Renderer requests (render, streaming render, asset upload, and dev-server asset fetch), implemented via a newRendererHttpClientwith explicitTimeoutError/ConnectionError/HTTPErrorhandling and multipart/form/JSON body builders.Configuration semantics shift:
renderer_http_pool_sizebecomes a per-request HTTP/2 stream limit (with warnings when set),renderer_http_pool_timeoutis treated as a TCP connect timeout, andrenderer_http_keep_alive_timeoutis retained but effectively deprecated (warns when set) because clients are now request-scoped (no cross-request connection reuse yet).Updates streaming behavior and tests/mocks to be HTTPX-independent, including a fix for duplicated newline-prefixed chunks and new first-chunk latency warning logic; also updates docs/changelog and bumps Pro’s required Ruby to 3.3+ with new gem dependencies (
async-http,io-endpoint).Reviewed by Cursor Bugbot for commit fc31472. Bugbot is set up for automated code reviews on this repo. Configure here.