Skip to content

Ensure connection is closed when raw SSE stream raises#248

Merged
rsamoilov merged 2 commits intorage-rb:mainfrom
jsxs0:fix-raw-stream-connection-leak
Mar 30, 2026
Merged

Ensure connection is closed when raw SSE stream raises#248
rsamoilov merged 2 commits intorage-rb:mainfrom
jsxs0:fix-raw-stream-connection-leak

Conversation

@jsxs0
Copy link
Copy Markdown
Contributor

@jsxs0 jsxs0 commented Mar 26, 2026

Summary

Add a rescue block to SSE::Application#start_raw_stream, so the Iodine connection is closed when the user's Proc raises an exception.

Problem

start_formatted_stream (Enumerator path) has ensure connection.close; the connection always cleans up, even on exception. start_raw_stream (Proc path) does not — if the Proc raises before calling connection.close, the connection leaks until Iodine's socket timeout.

# Has ensure — connection always closed
def start_formatted_stream(connection)
  @stream.each { |event| ... }
ensure
  connection.close
end

# Missing cleanup — connection leaks on exception
def start_raw_stream(connection)
  @stream.call(Rage::SSE::ConnectionProxy.new(connection))
end

Why not ensure?

Raw SSE streams can be async; the proc may spawn background fibers and return early while the connection stays alive (e.g. Datastar SDK). Using ensure would close the connection before background fibers can write to it.

Fix

def start_raw_stream(connection)
  @stream.call(Rage::SSE::ConnectionProxy.new(connection))
rescue => e
  connection.close if connection.open?
  raise e
end

Closes only on exception. The connection.open? guard avoids double-closing when the Proc already called close before raising.

Test plan

Added spec/sse/application_spec.rb with 3 test cases:

  • Proc raises → connection closed
  • Proc returns without closing (async pattern) → connection stays open
  • Proc closes itself → no interference

bundle exec rspec spec/sse/ — 47 examples, 0 failures

Add an `ensure` block to `start_raw_stream` so the connection is
always closed if the user's Proc raises an exception without
calling `close`. This matches the cleanup behavior already present
in `start_formatted_stream`, which has `ensure connection.close`.

Without this fix, a Proc that raises before closing leaves the
Iodine connection open until socket timeout.
Copy link
Copy Markdown
Member

@rsamoilov rsamoilov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jsxs0,

Thank you for the contribution! Unfortunately, this commit is a breaking change as it makes a reasonable but incorrect assumption about the synchronous nature of raw SSE streams.

It's a completely valid approach to run the stream on a separate fiber or thread. In such case, the proc that's passed to render sse: finishes executing almost immediately but long before the background fiber/thread has a chance to do the work and write to the connection.

If you check your changes against datastar-example, you'll see how the integration breaks - the Datastar SDK executes streams in separate fibers allowing the SSE stream proc to finish early. Adding the ensure block means the connection is closed before anything has been written to it.

However, if we focus on closing the connection when the SSE stream raises, you idea makes sense and actually fixes the existing issue. We just need to ensure the the connection is closed only on exception.

Address review feedback: the ensure block breaks async patterns
where the proc spawns background fibers and returns early (e.g.
Datastar SDK). Changed to rescue so the connection is only closed
when the proc raises, not when it completes normally.

Updated tests:
- Proc raises → connection closed
- Proc returns without closing (async pattern) → connection stays open
- Proc closes itself → no interference
@jsxs0
Copy link
Copy Markdown
Contributor Author

jsxs0 commented Mar 28, 2026

Hi, @rsamoilov.

Updated in 819f747, switched from ensure to rescue so the connection is only closed on exception. The proc returning early (async/fiber pattern) no longer triggers cleanup.

Updated tests:

  • Proc raises -> connection closed
  • Proc returns without closing (async pattern) -> connection stays open
  • Proc closes itself -> no interference

All 12 CI checks pass. Will also verify against datastar-example to confirm the Datastar SDK integration works correctly and update here.

@jsxs0
Copy link
Copy Markdown
Contributor Author

jsxs0 commented Mar 28, 2026

Verified against datastar-example, pointed its Gemfile at the local Rage branch with the rescue fix.

The Datastar SDK streams all 13 SSE events correctly (H, He, Hel... Hello, world!).

The connection stays open while background fibers write, and closes normally after completion.

@jsxs0 jsxs0 requested a review from rsamoilov March 28, 2026 16:17
Copy link
Copy Markdown
Member

@rsamoilov rsamoilov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@rsamoilov rsamoilov merged commit 3e4380b into rage-rb:main Mar 30, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants