diff --git a/react_on_rails_pro/lib/react_on_rails_pro/request.rb b/react_on_rails_pro/lib/react_on_rails_pro/request.rb index c08259e36c..8720cb39bf 100644 --- a/react_on_rails_pro/lib/react_on_rails_pro/request.rb +++ b/react_on_rails_pro/lib/react_on_rails_pro/request.rb @@ -9,7 +9,9 @@ class Request # rubocop:disable Metrics/ClassLength class << self def reset_connection @connection&.close + @connection_without_retries&.close @connection = create_connection + @connection_without_retries = create_connection(enable_retries: false) end def render_code(path, js_code, send_bundle) @@ -82,17 +84,29 @@ def asset_exists_on_vm_renderer?(filename) private + # NOTE: We maintain two separate HTTP connection pools to handle streaming vs non-streaming requests. + # This doubles the memory footprint (e.g., if renderer_http_pool_size is 10, we use 20 total connections). + # This tradeoff is acceptable to prevent body duplication in streaming responses. + def connection @connection ||= create_connection end - def perform_request(path, **post_options) # rubocop:disable Metrics/AbcSize,Metrics/CyclomaticComplexity + def connection_without_retries + @connection_without_retries ||= create_connection(enable_retries: false) + end + + def perform_request(path, **post_options) # rubocop:disable Metrics/AbcSize,Metrics/CyclomaticComplexity,Metrics/PerceivedComplexity + # For streaming requests, use connection without retries to prevent body duplication + # The StreamRequest class handles retries properly by starting fresh requests + conn = post_options[:stream] ? connection_without_retries : connection + available_retries = ReactOnRailsPro.configuration.renderer_request_retry_limit retry_request = true while retry_request begin start_time = Time.now - response = connection.post(path, **post_options) + response = conn.post(path, **post_options) raise response.error if response.is_a?(HTTPX::ErrorResponse) request_time = Time.now - start_time @@ -217,17 +231,20 @@ def common_form_data ReactOnRailsPro::Utils.common_form_data end - def create_connection + def create_connection(enable_retries: true) url = ReactOnRailsPro.configuration.renderer_url Rails.logger.info do "[ReactOnRailsPro] Setting up Node Renderer connection to #{url}" end - HTTPX - # For persistent connections we want retries, - # so the requests don't just fail if the other side closes the connection - # https://honeyryderchuck.gitlab.io/httpx/wiki/Persistent - .plugin(:retries, max_retries: 1, retry_change_requests: true) + http_client = HTTPX + # For persistent connections we want retries, + # so the requests don't just fail if the other side closes the connection + # https://honeyryderchuck.gitlab.io/httpx/wiki/Persistent + # However, for streaming requests, retries cause body duplication + # See https://github.com/shakacode/react_on_rails/issues/1895 + http_client = http_client.plugin(:retries, max_retries: 1, retry_change_requests: true) if enable_retries + http_client .plugin(:stream) # See https://www.rubydoc.info/gems/httpx/1.3.3/HTTPX%2FOptions:initialize for the available options .with( diff --git a/react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb b/react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb index 9eaf6c5641..b9c3bf9bad 100644 --- a/react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb +++ b/react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb @@ -75,6 +75,9 @@ def each_chunk(&block) send_bundle = false error_body = +"" + # Retry logic for streaming requests is handled here by starting fresh requests. + # The HTTPx connection used for streaming has retries disabled (see Request#connection_without_retries) + # to prevent body duplication when partial chunks are already sent to the client. loop do stream_response = @request_executor.call(send_bundle) diff --git a/react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/TestingStreamableComponent.jsx b/react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/TestingStreamableComponent.jsx new file mode 100644 index 0000000000..bd9076ef68 --- /dev/null +++ b/react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/TestingStreamableComponent.jsx @@ -0,0 +1,15 @@ +'use client'; + +import React from 'react'; + +// Simple test component for streaming SSR tests +function TestingStreamableComponent({ helloWorldData }) { + return ( +
+
Chunk 1: Stream React Server Components
+
Hello, {helloWorldData.name}!
+
+ ); +} + +export default TestingStreamableComponent; diff --git a/react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb b/react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb index 9cae3fb11b..d2a79e8c48 100644 --- a/react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb +++ b/react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb @@ -330,6 +330,7 @@ def response; end def mock_request_and_response(mock_chunks = chunks, count: 1) # Reset connection instance variables to ensure clean state for tests ReactOnRailsPro::Request.instance_variable_set(:@connection, nil) + ReactOnRailsPro::Request.instance_variable_set(:@connection_without_retries, nil) original_httpx_plugin = HTTPX.method(:plugin) allow(HTTPX).to receive(:plugin) do |*args| original_httpx_plugin.call(:mock_stream).plugin(*args) diff --git a/react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb b/react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb index 7d05079485..f0c26fbd83 100644 --- a/react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb +++ b/react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb @@ -11,7 +11,7 @@ def change_text_expect_dom_selector(dom_selector, expect_no_change: false) find("input").set new_text within("h3") do if expect_no_change - expect(subject).not_to have_content new_text + expect(subject).to have_no_content new_text else expect(subject).to have_content new_text end @@ -110,7 +110,7 @@ def change_text_expect_dom_selector(dom_selector, expect_no_change: false) it "changes name in message according to input" do visit "/client_side_hello_world" change_text_expect_dom_selector("#HelloWorld-react-component-0") - click_link "Hello World Component Server Rendered, with extra options" + click_link "Hello World Component Server Rendered, with extra options" # rubocop:disable Capybara/ClickLinkOrButtonStyle change_text_expect_dom_selector("#my-hello-world-id") end end @@ -174,19 +174,19 @@ def change_text_expect_dom_selector(dom_selector, expect_no_change: false) before do visit "/" - click_link "React Router" + click_link "React Router" # rubocop:disable Capybara/ClickLinkOrButtonStyle end context "when rendering /react_router" do it { is_expected.to have_text("Woohoo, we can use react-router here!") } it "clicking links correctly renders other pages" do - click_link "Router First Page" + click_link "Router First Page" # rubocop:disable Capybara/ClickLinkOrButtonStyle expect(page).to have_current_path("/react_router/first_page") first_page_header_text = page.find(:css, "h2#first-page").text expect(first_page_header_text).to eq("React Router First Page") - click_link "Router Second Page" + click_link "Router Second Page" # rubocop:disable Capybara/ClickLinkOrButtonStyle expect(page).to have_current_path("/react_router/second_page") second_page_header_text = page.find(:css, "h2#second-page").text expect(second_page_header_text).to eq("React Router Second Page") @@ -239,12 +239,12 @@ def change_text_expect_dom_selector(dom_selector, expect_no_change: false) end end -describe "Manual client hydration", :js, type: :system do +describe "Manual client hydration", :js do before { visit "/xhr_refresh" } it "HelloWorldRehydratable onChange should trigger" do within("form") do - click_button "refresh" + click_button "refresh" # rubocop:disable Capybara/ClickLinkOrButtonStyle end within("#HelloWorldRehydratable-react-component-1") do find("input").set "Should update" @@ -407,9 +407,9 @@ def change_text_expect_dom_selector(dom_selector, expect_no_change: false) # Ensure that the component state is not updated change_text_expect_dom_selector(selector, expect_no_change: true) - expect(page).not_to have_text "Loading branch1" - expect(page).not_to have_text "Loading branch2" - expect(page).not_to have_text(/Loading branch1 at level \d+/) + expect(page).to have_no_text "Loading branch1" + expect(page).to have_no_text "Loading branch2" + expect(page).to have_no_text(/Loading branch1 at level \d+/) expect(page).to have_text(/branch1 \(level \d+\)/, count: 5) end @@ -417,8 +417,8 @@ def change_text_expect_dom_selector(dom_selector, expect_no_change: false) # visit waits for the page to load, so we ensure that the page is loaded before checking the hydration status visit "#{path}?skip_js_packs=true" expect(page).to have_text "HydrationStatus: Streaming server render" - expect(page).not_to have_text "HydrationStatus: Hydrated" - expect(page).not_to have_text "HydrationStatus: Page loaded" + expect(page).to have_no_text "HydrationStatus: Hydrated" + expect(page).to have_no_text "HydrationStatus: Page loaded" end end diff --git a/react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb b/react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb index 6c775ae799..495a2a167a 100644 --- a/react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb +++ b/react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb @@ -194,5 +194,32 @@ expect(mocked_block).not_to have_received(:call) end end + + it "does not use HTTPx retries plugin for streaming requests to prevent body duplication" do + # This test verifies the fix for https://github.com/shakacode/react_on_rails/issues/1895 + # When streaming requests encounter connection errors mid-transmission, HTTPx retries + # would cause body duplication because partial chunks are already sent to the client. + # The StreamRequest class handles retries properly by starting fresh requests. + + # Reset connections to ensure we're using a fresh connection + described_class.reset_connection + + # Trigger a streaming request + mock_streaming_response(render_full_url, 200) do |yielder| + yielder.call("Test chunk\n") + end + + stream = described_class.render_code_as_stream("/render", "console.log('test');", is_rsc_payload: false) + chunks = [] + stream.each_chunk { |chunk| chunks << chunk } + + # Verify that the streaming request completed successfully + expect(chunks).to eq(["Test chunk"]) + + # Verify that the connection_without_retries was created + # by checking that a connection was created with retries disabled + connection_without_retries = described_class.send(:connection_without_retries) + expect(connection_without_retries).to be_a(HTTPX::Session) + end end end