The Rainbows server isn't maintained so IMO we shouldn't use it for example or running integration tests with it (if you agree I can remove them in another PR)

You can look at my branch where I switched the integration tests to Puma (and kept Rainbows for when Rack 2 is used): master...dentarg:sinatra:rack-3

Here I've updated the chat example, so Thin is used instead. Thin was deprecated some time ago because work on it stopped, but it's being updated again and even macournoyer/thin#390 is being worked on.

I really think we should use Puma in the examples, not Thin. I recall that being discussed before and I think there was agreement from @jkowens

I agree examples should typically use Puma, but I'm not sure Puma will work for that chat example. I think the streaming :keep_open option requires Event Machine, so Thin might be necessary.

#1035
#604

Exactly, the chat example doesn't work with Puma, that's why I used Thin instead.

You can look at my branch where I switched the integration tests to Puma (and kept Rainbows for when Rack 2 is used): master...dentarg:sinatra:rack-3

Nice! Didn't know you were working on it. What do you think on removing Rainbows completely? Do you think is worth to run tests with it?

I'd be in favor or removing Rainbows completely if others agree 🧹 ✨

Me too! ⬆️

I agree examples should typically use Puma, but I'm not sure Puma will work for that chat example. I think the streaming :keep_open option requires Event Machine, so Thin might be necessary.

We can always update the examples to not use :keep_open. Haven't read all the old issues but #1035 had a reference to Smashing/smashing#93 that linked the following diff that changes from Thin to Puma for an event driven app: Shopify/dashing@master...puma_webserver#diff-1d8825da15797246388a6cbd25a170fbd2203a1b1cb6fd25f0d601e3c50d8ba4 – it looks resonable to me, but needs to be tried of course

@dentarg I think maybe we could also consider a similar implementation as Roda:

https://github.com/jeremyevans/roda/blob/c919ac9c7e88f159f8c6185b0bdb5a9311e24d93/lib/roda/plugins/streaming.rb#L143

What we have doesn't seem too far off.

master...jkowens:sinatra:update-streaming

Sure, that sounds great. I tried your branch locally, ./examples/chat.rb (uninstalled rack 3.0.0 first) but I had to kill -6 puma, it didn't respond to ^C (which works on master with rainbows). Maybe not related to the streaming, I need to play more with all this to understand it better myself.

Sure, that sounds great. I tried your branch locally, ./examples/chat.rb (uninstalled rack 3.0.0 first) but I had to kill -6 puma, it didn't respond to ^C (which works on master with rainbows). Maybe not related to the streaming, I need to play more with all this to understand it better myself.

Yeah I had the same issue shutting down the server. Puma won’t shut down if there are open connections. I think there would have to be code to trap SIGINT and close all the open connections, or possibly have a heartbeat event that will close stale connections.

Thread.new do
  loop do
    connections.each do |out|
      out << "heartbeat:\n"
    rescue
      out.close
    end
    sleep 5
  end
end

so Thin might be necessary

Thin hasn't been updated to support Rack 3: https://github.com/macournoyer/thin/blob/v1.8.1/thin.gemspec#L25 so I think we should to find a way to use Puma or Falcon or drop these examples or something

@jkowens Can you walk me through the changes in your branch? I don't think I'm following them fully

diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
index 63a70c75..03b41c1c 100644
--- a/lib/sinatra/base.rb
+++ b/lib/sinatra/base.rb
@@ -477,8 +477,9 @@ module Sinatra
             @back.call(self)
           rescue Exception => e
             @scheduler.schedule { raise e }
+          ensure
+            close
           end
-          close unless @keep_open
         end
       end

@@ -509,7 +510,16 @@ module Sinatra
     def stream(keep_open = false)
       scheduler = env['async.callback'] ? EventMachine : Stream
       current   = @params.dup
-      body Stream.new(scheduler, keep_open) { |out| with_params(current) { yield(out) } }
+      block = if scheduler == Stream && keep_open
+        proc do |out|
+          until out.closed?
+            with_params(current) { yield(out) }
+          end
+        end
+      else
+        proc { |out| with_params(current) { yield(out) } }
+      end
+      body Stream.new(scheduler, keep_open, &block)
     end

     # Specify response freshness policy for HTTP caches (Cache-Control header).

I tried those changes and the chat example as a modular Sinatra app

# app.rb
require 'sinatra/base'

$connections = Set.new

class App < Sinatra::Base
  get '/' do
    halt erb(:login) unless params[:user]
    erb :chat, locals: { user: params[:user].gsub(/\W/, '') }
  end

  get '/stream', provides: 'text/event-stream' do
    stream :keep_open do |out|
      $connections << out
      out.callback { $connections.delete(out) }
      sleep 1
    end
  end

  post '/' do
    $connections.each do |out|
      out << "data: #{params[:msg]}\n\n"
    rescue
      out.close
    end
    204 # response without entity body
  end
end

# config.ru
require_relative "app"

run App

and I started Puma 6.0.1 with bundle exec puma and after connecting to it (with Chrome) I can not stop it with ^C, not even efter I closed my browser tabs. Not sure what's going on, feels a bit like a bug in Puma, that you can create an app making the server unresponsive. In Puma cluster mode (-w 2) Puma is able to shut down workers if I'm patient. Of course, the chat app isn't as fun then (hehe 😄)

@dentarg I modified that branch a bit to clean up some issues. For app servers that don't use EventMachine, it creates a loop to hold the connection open (when keep_open specified). I also modified the chat example to send a "heartbeat". That way if the browser window is closed, the connection should be closed and Puma will be able to shut down. I tried to test it with Falcon too, but it doesn't seem to work. I'm not sure if Falcon buffers the response or if I'm just doing something wrong 😵‍💫

@epergo I merged #1858 that updated the chat example so if you have time to just remove Rainbows that would be super :)

@epergo I merged #1858 that updated the chat example so if you have time to just remove Rainbows that would be super :)

Nice! Going to close this one then and I'll open a new one when ready