luckyframework / lucky

A full-featured Crystal web framework that catches bugs for you, runs incredibly fast, and helps you write code that lasts.
https://luckyframework.org
MIT License
2.57k stars 156 forks source link

SSE Watcher issues #1744

Closed robacarp closed 1 year ago

robacarp commented 1 year ago

Describe the bug

This could pretty easily fall into the simple territory of a bug we're okay living with.

In development mode, with lucky watch --watcher=sse, each time a browser is connected to the SSE server, a new loop is initiated with sleep 0.1s.

Simply clicking around on a site, navigating around pages, it's easy to get this loop spinning dozens of times.

I think this would cause an issue if the lucy dev server is running for a long time, and active development is refreshing the page over and over.

To Reproduce Steps to reproduce the behavior:

Example:

  1. Modify Procfile.dev to activate the SSE reload server.
    - web: .lucky watch --reload-browser
    + web: .lucky watch --reload-browser --watcher=sse
  2. Start server and poke around on your project.
  3. What happens? I don't know!

Screenshots/code

I was able to prove that this happens by modifying lib/lucky/tasks/watch.cr like this. After modifying the file, rm bin/lucky.watch and then lucky dev will compile the task on and run it.

    def initialize
      @server = HTTP::Server.new do |context|
        context.response.headers.merge!({
          "Access-Control-Allow-Origin" => "*",
          "Content-Type"                => "text/event-stream",
          "Connection"                  => "keep-alive",
          "Cache-Control"               => "no-cache",
        })
        context.response.status_code = 200

+        puts "SSE Activated".colorize(:green)
+        n = rand(1000000)

        # SSE start
        loop do
+          puts "SSE Loop #{n}"
          if @should_restart
+            puts "Triggering SSE reload"
            context.response.print "data: update\n\n"
            context.response.flush
            @should_restart = false
            break
          end
          sleep 0.1
        end

+        puts "SSE Deactivated".colorize(:red)
        # SSE stop
      end
    end
Very Verbose Output:
web     | GET /sign_in (84038b3c-298d-41fe-bb41-6a18f22299c4)
web     |  ▸ Handled by SignIns::New
web     |  ▸ Rendered SignIns::NewPage
web     |  ▸ Sent 200 OK (13.22ms) (84038b3c-298d-41fe-bb41-6a18f22299c4)
web     | SSE Activated
web     | SSE Loop 749326
web     | SSE Loop 749326
web     | SSE Loop 749326
web     | SSE Loop 749326
web     | SSE Loop 749326
web     | SSE Loop 749326
web     | SSE Loop 749326
web     | SSE Loop 749326
web     | SSE Loop 749326
web     | 
web     | GET /sign_in (08675d9f-fd18-4aeb-a0f1-f3b155377843)
web     |  ▸ Handled by SignIns::New
web     |  ▸ Rendered SignIns::NewPage
web     |  ▸ Sent 200 OK (1.59ms) (08675d9f-fd18-4aeb-a0f1-f3b155377843)
web     | SSE Loop 749326
web     | SSE Activated
web     | SSE Loop 876529
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | 
web     | GET /sign_in (4e3f6485-a3e7-4b4d-85d3-87d3b4599666)
web     |  ▸ Handled by SignIns::New
web     |  ▸ Rendered SignIns::NewPage
web     |  ▸ Sent 200 OK (1.54ms) (4e3f6485-a3e7-4b4d-85d3-87d3b4599666)
web     | SSE Activated
web     | SSE Loop 944547
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | 
web     | GET /sign_in (925aa16b-dbe9-4386-8b4e-70e1b28d3650)
web     |  ▸ Handled by SignIns::New
web     |  ▸ Rendered SignIns::NewPage
web     |  ▸ Sent 200 OK (1.58ms) (925aa16b-dbe9-4386-8b4e-70e1b28d3650)
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Activated
web     | SSE Loop 148330
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 148330
web     | 
web     | GET /sign_in (dbe8ad87-07cd-4815-a8ba-0636b8b2c82d)
web     |  ▸ Handled by SignIns::New
web     |  ▸ Rendered SignIns::NewPage
web     |  ▸ Sent 200 OK (1.93ms) (dbe8ad87-07cd-4815-a8ba-0636b8b2c82d)
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 148330
web     | SSE Activated
web     | SSE Loop 551843
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 148330
web     | SSE Loop 551843
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 148330
web     | 
web     | GET /sign_in (b977bd10-53a9-462d-aa1d-743132d2e0df)
web     |  ▸ Handled by SignIns::New
web     |  ▸ Rendered SignIns::NewPage
web     |  ▸ Sent 200 OK (2.09ms) (b977bd10-53a9-462d-aa1d-743132d2e0df)
web     | SSE Loop 551843
web     | SSE Activated
web     | SSE Loop 26637
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 148330
web     | SSE Loop 551843
web     | SSE Loop 26637
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 148330
web     | SSE Loop 551843
web     | 
web     | GET /sign_in (21537ee9-4d86-435e-8dfc-e371da61affb)
web     |  ▸ Handled by SignIns::New
web     |  ▸ Rendered SignIns::NewPage
web     |  ▸ Sent 200 OK (1.51ms) (21537ee9-4d86-435e-8dfc-e371da61affb)
web     | SSE Loop 26637
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 148330
web     | SSE Loop 551843
web     | SSE Loop 26637
web     | 
web     | GET /sign_in (cab4e25c-c142-4bae-b40e-4f6ddda306a9)
web     |  ▸ Handled by SignIns::New
web     |  ▸ Rendered SignIns::NewPage
web     |  ▸ Sent 200 OK (1.56ms) (cab4e25c-c142-4bae-b40e-4f6ddda306a9)
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 148330
web     | SSE Loop 551843
web     | SSE Loop 26637
web     | SSE Activated
web     | SSE Loop 724780
web     | SSE Loop 749326
web     | SSE Loop 876529
web     | SSE Loop 944547
web     | SSE Loop 148330
web     | SSE Loop 551843

Versions (please complete the following information):

robacarp commented 1 year ago

Also perhaps relevant, the Mozilla docs on SSE contain this warning. I don't think Crystal has any packaged HTTP2 support yet.

Warning: When not used over HTTP/2, SSE suffers from a limitation to the maximum number of open connections, which can be especially painful when opening multiple tabs, as the limit is per browser and is set to a very low number (6). The issue has been marked as "Won't fix" in Chrome and Firefox. This limit is per browser + domain, which means that you can open 6 SSE connections across all of the tabs to www.example1.com and another 6 SSE connections to www.example2.com (per Stackoverflow). When using HTTP/2, the maximum number of simultaneous HTTP streams is negotiated between the server and the client (defaults to 100).

robacarp commented 1 year ago

As well, I've just come across this. It appears that, since even after disconnect the loop still spins, when a reload event is triggered every open connection which is no longer tied to the browser throws an exception.

After the exception is thrown, the loop is obviously terminated. Which (accidentally) mitigates this bug at least a bit.

image

robacarp commented 1 year ago

image

I witnessed this behavior just now. If I boot the server, and then trigger a recompile before loading a page with the browser, the first time I do the page is immediately reloaded.

mdwagner commented 1 year ago

Ah ok, I guess my implementation was a little naive when I thought just changing @should_restart would work across all requests (fibers?). I did not notice this in my initial testing, but it's definitely a bug.

I was already aware of the limitations of SSE, but I know some people swear by it, so I thought it was worth keeping around.

I think my new implementation would be to store each request Context in a list, that when a reload should happen, it just pops each one off and runs the reload logic, then only break the loop when the context is closed. We'll see if that works.

mdwagner commented 1 year ago

@robacarp would you be willing to try out some of my fixes with your local setup?