Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSE Watcher issues #1744

Closed
robacarp opened this issue Sep 30, 2022 · 5 comments · Fixed by #1748
Closed

SSE Watcher issues #1744

robacarp opened this issue Sep 30, 2022 · 5 comments · Fixed by #1748
Labels

Comments

@robacarp
Copy link
Contributor

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
  1. Start server and poke around on your project.
  2. 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):

  • Lucky version 1.0.0-rc1
  • Crystal version 1.5.1
@robacarp robacarp added the bug label Sep 30, 2022
@robacarp
Copy link
Contributor Author

robacarp commented Sep 30, 2022

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 robacarp changed the title Could the SSE Watcher go into a spin lock? SSE Watcher issues Sep 30, 2022
@robacarp
Copy link
Contributor Author

robacarp commented Sep 30, 2022

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

@jwoertink jwoertink added the hacktoberfest Valid Issue for Hacktoberfest label Sep 30, 2022
@robacarp
Copy link
Contributor Author

robacarp commented Oct 1, 2022

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
Copy link
Contributor

mdwagner commented Oct 5, 2022

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
Copy link
Contributor

mdwagner commented Oct 5, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants