From 683f3e1ab6dea55b4e02ca59b1c4acc5fb54e984 Mon Sep 17 00:00:00 2001
From: Cezary Baginski <cezary@chronomantic.net>
Date: Mon, 26 Oct 2015 23:35:33 +0100
Subject: [PATCH 1/7] rework sockets to prevent threading/race issues

---
 lib/guard/livereload/reactor.rb           | 46 ++++++++++---
 lib/guard/livereload/reactor/sockets.rb   | 35 ++++++++++
 spec/lib/guard/livereload/reactor_spec.rb | 81 +++++++++++++++++------
 3 files changed, 130 insertions(+), 32 deletions(-)
 create mode 100644 lib/guard/livereload/reactor/sockets.rb

diff --git a/lib/guard/livereload/reactor.rb b/lib/guard/livereload/reactor.rb
index 1a3e1cc..3b1332b 100644
--- a/lib/guard/livereload/reactor.rb
+++ b/lib/guard/livereload/reactor.rb
@@ -1,17 +1,39 @@
 require 'multi_json'
 
+require 'guard/livereload/reactor/sockets'
+
 module Guard
   class LiveReload < Plugin
     class Reactor
-      attr_reader :web_sockets, :thread, :options, :connections_count
+
+      attr_reader :thread, :options
 
       def initialize(options)
-        @web_sockets       = []
-        @options           = options
-        @thread            = Thread.new { _start_reactor }
+
+        @sockets = Sockets.new
+        @options = options
+        @thread = Thread.new { _start_reactor }
         @connections_count = 0
       end
 
+      def sockets
+        @sockets
+      end
+
+      # TODO: deprecate
+      # just to prevent semver change
+      # (it's used only for tests anyway)
+      def web_sockets
+        sockets.internal_list
+      end
+
+      # TODO: deprecate
+      # just to prevent semver change
+      # (it's used only for tests anyway)
+      def connections_count
+        sockets.internal_list.size
+      end
+
       def stop
         thread.kill
       end
@@ -26,7 +48,8 @@ def reload_browser(paths = [])
         paths.each do |path|
           data = _data(path)
           Compat::UI.debug(data)
-          web_sockets.each { |ws| ws.send(MultiJson.encode(data)) }
+          json = MultiJson.encode(data)
+          sockets.broadcast(json)
         end
       end
 
@@ -63,22 +86,25 @@ def _start_reactor
       end
 
       def _connect(ws)
-        @connections_count += 1
-        Compat::UI.info 'Browser connected.' if connections_count == 1
+        Compat::UI.info 'New browser connecting...'
 
-        ws.send MultiJson.encode(
+        json = MultiJson.encode(
           command:    'hello',
           protocols:  ['http://livereload.com/protocols/official-7'],
           serverName: 'guard-livereload'
         )
-        @web_sockets << ws
+
+        sockets.add(ws) { |socket| socket.send(json) }
+
+        Compat::UI.info "New browser connected (total: #{connections_count})"
       rescue
         Compat::UI.error $!
         Compat::UI.error $!.backtrace
       end
 
       def _disconnect(ws)
-        @web_sockets.delete(ws)
+        sockets.delete(ws)
+        Compat::UI.info "Browser disconnected (total: #{connections_count})"
       end
 
       def _print_message(message)
diff --git a/lib/guard/livereload/reactor/sockets.rb b/lib/guard/livereload/reactor/sockets.rb
new file mode 100644
index 0000000..82a0457
--- /dev/null
+++ b/lib/guard/livereload/reactor/sockets.rb
@@ -0,0 +1,35 @@
+require 'thread'
+
+module Guard
+  class LiveReload < Plugin
+    class Reactor
+      class Sockets
+        def initialize
+          @sockets = []
+          @mutex = Mutex.new
+        end
+
+        def internal_list
+          @mutex.synchronize { @sockets }
+        end
+
+        def broadcast(json)
+          @mutex.synchronize do
+            @sockets.each { |ws| ws.send(json) }
+          end
+        end
+
+        def add(socket)
+          @mutex.synchronize do
+            yield socket if block_given? # within mutex, so 'hello' has a chance to be sent
+            @sockets << socket
+          end
+        end
+
+        def delete(socket)
+          @mutex.synchronize { @sockets.delete(socket) }
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/guard/livereload/reactor_spec.rb b/spec/lib/guard/livereload/reactor_spec.rb
index a48aed9..ced3123 100644
--- a/spec/lib/guard/livereload/reactor_spec.rb
+++ b/spec/lib/guard/livereload/reactor_spec.rb
@@ -1,14 +1,22 @@
 RSpec.describe Guard::LiveReload::Reactor do
   let(:paths) { %w[stylesheets/layout.css stylesheets/style.css] }
 
+  let(:sockets) { instance_double(described_class::Sockets) }
+
   before do
     allow(Guard::Compat::UI).to receive(:info)
     allow(Guard::Compat::UI).to receive(:debug)
     allow(Guard::Compat::UI).to receive(:error)
     allow(Guard::Compat::UI).to receive(:warning)
+
+    allow(described_class::Sockets).to receive(:new).and_return(sockets)
   end
 
   describe '#reload_browser(paths = [])' do
+    before do
+      allow(sockets).to receive(:broadcast)
+    end
+
     it 'displays a message' do
       expect(Guard::Compat::UI).to receive(:info).
         with('Reloading browser: stylesheets/layout.css stylesheets/style.css')
@@ -26,46 +34,75 @@
       new_live_reactor(notify: true).reload_browser(paths)
     end
 
-    it 'each web socket receives send with data containing default options for each path modified' do
-      reactor = new_live_reactor
-      paths.each do |path|
-        reactor.web_sockets.each do |ws|
-          expect(ws).to receive(:send).with(MultiJson.encode(['refresh', path: "#{Dir.pwd}/#{path}", apply_js_live: true, apply_css_live: true]))
+    context 'with multiple paths' do
+      subject { new_live_reactor(*args) }
+
+      let(:paths) { %w(foo.css bar.css) }
+
+      context 'with default options' do
+        let(:options) { { apply_css_live: false } }
+        let(:args) { [] }
+
+        it 'sends json data with options' do
+          paths.each do |path|
+            json = MultiJson.encode('command': 'reload', path: "#{Dir.pwd}/#{path}", liveCSS: true)
+            expect(sockets).to receive(:broadcast).with(json)
+          end
+          subject.reload_browser(paths)
         end
       end
-      reactor.reload_browser(paths)
-    end
 
-    it 'each web socket receives send with data containing custom options for each path modified' do
-      reactor = new_live_reactor(apply_css_live: false, apply_js_live: false)
-      paths.each do |path|
-        reactor.web_sockets.each do |ws|
-          expect(ws).to receive(:send).with(MultiJson.encode(['refresh', path: "#{Dir.pwd}/#{path}", apply_js_live: false, apply_css_live: false]))
+      context 'with custom options' do
+        let(:options) { { apply_css_live: false } }
+        let(:args) { [options] }
+
+        it 'sends json data with custom options' do
+          paths.each do |path|
+            json = MultiJson.encode('command': 'reload', path: "#{Dir.pwd}/#{path}", liveCSS: false)
+            expect(sockets).to receive(:broadcast).with(json)
+          end
+          subject.reload_browser(paths)
         end
       end
-      reactor.reload_browser(paths)
     end
   end
 
   describe '#_connect(ws)' do
     let(:ws)      { double.as_null_object }
     let(:reactor) { new_live_reactor }
+    let!(:socket_list) { [] }
+
+    before do
+      allow(sockets).to receive(:add).with(ws) do |socket|
+        socket_list << socket
+      end
 
-    it 'displays a message once' do
-      expect(Guard::Compat::UI).to receive(:info).with('Browser connected.').once
-      reactor.send(:_connect, ws)
-      reactor.send(:_connect, ws)
+      allow(sockets).to receive(:internal_list).and_return(socket_list)
     end
 
-    it 'increments the connection count' do
-      expect do
+    context 'when connected once' do
+      it 'displays a message once' do
+        expect(Guard::Compat::UI).to receive(:info).with('New browser connecting...')
+        expect(Guard::Compat::UI).to receive(:info).with('New browser connected (total: 1)')
         reactor.send(:_connect, ws)
-      end.to change { reactor.connections_count }.from(0).to 1
+      end
     end
-  end
 
+    context 'when already connected' do
+      before do
+        allow(Guard::Compat::UI).to receive(:info)
+        reactor.send(:_connect, ws)
+      end
+
+      it 'displays a message with total' do
+        expect(Guard::Compat::UI).to receive(:info).with('New browser connecting...')
+        expect(Guard::Compat::UI).to receive(:info).with('New browser connected (total: 2)')
+        reactor.send(:_connect, ws)
+      end
+    end
+  end
 end
 
 def new_live_reactor(options = {})
-  Guard::LiveReload::Reactor.new({ api_version: '1.6', host: '0.0.0.0', port: '35729', apply_js_live: true, apply_css_live: true, grace_period: 0 }.merge(options))
+  Guard::LiveReload::Reactor.new({ host: '0.0.0.0', port: '35729', apply_css_live: true, grace_period: 0 }.merge(options))
 end

From 36d171363f5b82c92488d336c4f49ea277e41a5f Mon Sep 17 00:00:00 2001
From: Cezary Baginski <cezary@chronomantic.net>
Date: Mon, 26 Oct 2015 23:35:51 +0100
Subject: [PATCH 2/7] verify the options passed to reactor are known

---
 lib/guard/livereload/reactor.rb | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/lib/guard/livereload/reactor.rb b/lib/guard/livereload/reactor.rb
index 3b1332b..f9e46de 100644
--- a/lib/guard/livereload/reactor.rb
+++ b/lib/guard/livereload/reactor.rb
@@ -5,10 +5,32 @@
 module Guard
   class LiveReload < Plugin
     class Reactor
+      KNOWN_OPTIONS = %i(
+        notify
+        host
+        port
+        apply_css_live
+        override_url
+        grace_period
+
+        js_template
+        livereload_js_path
+
+        js_options
+
+        js_apple_webkit_extra_wait_time
+        js_default_extra_wait_time
+      )
+
+      # TODO: implement usage of js_options
+      # TODO: deprecate actual js options (js_apple(...) and js_default(...))
 
       attr_reader :thread, :options
 
       def initialize(options)
+        options.keys.each do |key|
+          fail "Unknown option: #{key.inspect}" unless KNOWN_OPTIONS.include?(key)
+        end
 
         @sockets = Sockets.new
         @options = options

From c02030b1f43c563f7389f9a7fa694e3533ac6cea Mon Sep 17 00:00:00 2001
From: Cezary Baginski <cezary@chronomantic.net>
Date: Tue, 27 Oct 2015 00:05:19 +0100
Subject: [PATCH 3/7] improve how JS options are set

---
 README.md                         | 13 ++++++-------
 js/livereload.js.erb              |  4 ++--
 lib/guard/livereload.rb           | 12 +++++++++++-
 spec/lib/guard/livereload_spec.rb | 23 ++++++++++++++++++++++-
 4 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/README.md b/README.md
index fe7c031..76f2b84 100644
--- a/README.md
+++ b/README.md
@@ -77,16 +77,15 @@ port: '12345'         # default '35729'
 apply_css_live: false # default true
 override_url: false   # default false
 grace_period: 0.5     # default 0 (seconds)
-js_template: './my_livereload.js.erb' # default is livereload.js.erb from gem
-```
 
-Additional custom JS template options (see livereload.js.erb for details):
-``` ruby
-js_apple_webkit_extra_wait_time: 50 # default is 5 (see issue #123)
-js_default_extra_wait_time: 100 # default is 200
+# Additional custom JS template options (see livereload.js.erb for details):
+js_template: './my_livereload.js.erb' # default is livereload.js.erb from gem
+js_options: {
+  apple_webkit_extra_wait_time: 50 # default is 5 (see issue #123)
+  default_extra_wait_time: 100 # default is 200
+}
 ```
 
-
 `notify` uses Guard's [system notifications](https://github.com/guard/guard/wiki/System-notifications).
 See [LiveReload configuration doc](https://github.com/mockko/livereload/blob/master/README-old.md) from version 1.x for more info about other options.
 
diff --git a/js/livereload.js.erb b/js/livereload.js.erb
index 12ac9cc..9a50b6c 100644
--- a/js/livereload.js.erb
+++ b/js/livereload.js.erb
@@ -733,9 +733,9 @@
          return this.waitUntilCssLoads(clone, function() {
            var additionalWaitingTime;
            if (/AppleWebKit/.test(navigator.userAgent)) {
-             additionalWaitingTime = <%= options[:js_apple_webkit_extra_wait_time] || 5 %>;
+             additionalWaitingTime = <%= options[:js_options][:apple_webkit_extra_wait_time] || 5 %>;
            } else {
-             additionalWaitingTime = <%= options[:js_default_extra_wait_time] || 200 %>;
+             additionalWaitingTime = <%= options[:js_options][:default_extra_wait_time] || 200 %>;
            }
            return _this.Timer.start(additionalWaitingTime, function() {
              var _ref;
diff --git a/lib/guard/livereload.rb b/lib/guard/livereload.rb
index 1ca1153..e926e0b 100644
--- a/lib/guard/livereload.rb
+++ b/lib/guard/livereload.rb
@@ -18,9 +18,19 @@ def initialize(options = {})
         apply_css_live: true,
         override_url:   false,
         grace_period:   0,
-        js_template: js_path
+        js_template: js_path,
+        js_options: {}
       }.merge(options)
 
+      %i(js_apple_webkit_extra_wait_time js_default_extra_wait_time).each do |key|
+        if options.keys.include?(key)
+          Guard::Compat::UI.deprecation "#{key.inspect} is deprecated. Check out :js_options parameter instead."
+          new_key = key.to_s.sub(/^js_/, '').to_sym
+          @options[:js_options][new_key] ||= options[key]
+          @options.delete(key)
+        end
+      end
+
       js_path = @options[:js_template]
 
       # NOTE: save snippet as instvar, so it's not GC'ed
diff --git a/spec/lib/guard/livereload_spec.rb b/spec/lib/guard/livereload_spec.rb
index 4a09ad0..ab64edc 100644
--- a/spec/lib/guard/livereload_spec.rb
+++ b/spec/lib/guard/livereload_spec.rb
@@ -113,6 +113,25 @@
         end
       end
     end
+
+    describe 'with deprecated JS option' do
+      subject { described_class.new(js_default_extra_wait_time: 123) }
+
+      before do
+        allow(Guard::Compat::UI).to receive(:deprecation)
+      end
+
+      it 'moves the value to the right field' do
+        expect(subject.options[:js_options]).to eq(default_extra_wait_time: 123)
+        expect(subject.options).to_not include(js_default_extra_wait_time: 123)
+      end
+
+      it 'shows a deprecation warning' do
+        expect(Guard::Compat::UI).to receive(:deprecation).with(/:js_default_extra_wait_time is deprecated. Check out :js_options/)
+        subject
+      end
+
+    end
   end
 
   describe '#start' do
@@ -125,6 +144,7 @@
         override_url:   false,
         grace_period:  0,
         js_template: '/foo/js/livereload.js.erb',
+        js_options: {},
         livereload_js_path: '/tmp/livereload-123'
       )
       plugin.start
@@ -139,7 +159,8 @@
         override_url:   true,
         grace_period:   1,
         js_template: '/foo/js/livereload.js.erb',
-        livereload_js_path: '/tmp/livereload-123'
+        js_options: {},
+        livereload_js_path: '/tmp/livereload-123',
       )
       plugin.start
     end

From 7bb33f1a6a972a4aa0c7e2b1d23a43e22e5df3d2 Mon Sep 17 00:00:00 2001
From: Cezary Baginski <cezary@chronomantic.net>
Date: Tue, 27 Oct 2015 00:26:15 +0100
Subject: [PATCH 4/7] remove options the reactor will not get

---
 lib/guard/livereload/reactor.rb | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/guard/livereload/reactor.rb b/lib/guard/livereload/reactor.rb
index f9e46de..f198103 100644
--- a/lib/guard/livereload/reactor.rb
+++ b/lib/guard/livereload/reactor.rb
@@ -17,9 +17,6 @@ class Reactor
         livereload_js_path
 
         js_options
-
-        js_apple_webkit_extra_wait_time
-        js_default_extra_wait_time
       )
 
       # TODO: implement usage of js_options

From b305d4e6ad4081c71c462b2f0aac0ede418a3661 Mon Sep 17 00:00:00 2001
From: Cezary Baginski <cezary@chronomantic.net>
Date: Tue, 27 Oct 2015 00:45:53 +0100
Subject: [PATCH 5/7] show warning when no browsers are connected

---
 lib/guard/livereload/reactor.rb           |  6 +++++-
 lib/guard/livereload/reactor/sockets.rb   |  4 ++++
 spec/lib/guard/livereload/reactor_spec.rb | 15 +++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/lib/guard/livereload/reactor.rb b/lib/guard/livereload/reactor.rb
index f198103..5e3958c 100644
--- a/lib/guard/livereload/reactor.rb
+++ b/lib/guard/livereload/reactor.rb
@@ -32,7 +32,6 @@ def initialize(options)
         @sockets = Sockets.new
         @options = options
         @thread = Thread.new { _start_reactor }
-        @connections_count = 0
       end
 
       def sockets
@@ -59,6 +58,11 @@ def stop
 
       def reload_browser(paths = [])
         msg = "Reloading browser: #{paths.join(' ')}"
+
+        if sockets.empty?
+          Compat::UI.warning "No browsers connected at this time! Browser won't refresh!"
+        end
+
         Compat::UI.info msg
         if options[:notify]
           Compat::UI.notify(msg, title: 'Reloading browser', image: :success)
diff --git a/lib/guard/livereload/reactor/sockets.rb b/lib/guard/livereload/reactor/sockets.rb
index 82a0457..15843e0 100644
--- a/lib/guard/livereload/reactor/sockets.rb
+++ b/lib/guard/livereload/reactor/sockets.rb
@@ -29,6 +29,10 @@ def add(socket)
         def delete(socket)
           @mutex.synchronize { @sockets.delete(socket) }
         end
+
+        def empty?
+          @mutex.synchronize { @sockets.empty? }
+        end
       end
     end
   end
diff --git a/spec/lib/guard/livereload/reactor_spec.rb b/spec/lib/guard/livereload/reactor_spec.rb
index ced3123..9acd912 100644
--- a/spec/lib/guard/livereload/reactor_spec.rb
+++ b/spec/lib/guard/livereload/reactor_spec.rb
@@ -15,6 +15,7 @@
   describe '#reload_browser(paths = [])' do
     before do
       allow(sockets).to receive(:broadcast)
+      allow(sockets).to receive(:empty?).and_return(false)
     end
 
     it 'displays a message' do
@@ -64,6 +65,20 @@
           subject.reload_browser(paths)
         end
       end
+
+    end
+
+    context 'with no connected browsers' do
+      subject { new_live_reactor }
+
+      before do
+        allow(sockets).to receive(:empty?).and_return(true)
+      end
+
+      it 'shows a warning' do
+        expect(Guard::Compat::UI).to receive(:warning).with(/No browsers connected at this time!/)
+        subject.reload_browser(paths)
+      end
     end
   end
 

From 6045cc382de7685e7801a0e67cb57bef97697e9d Mon Sep 17 00:00:00 2001
From: Cezary Baginski <cezary@chronomantic.net>
Date: Thu, 29 Oct 2015 23:37:24 +0100
Subject: [PATCH 6/7] implement Sockets#size

---
 lib/guard/livereload/reactor.rb           | 2 +-
 lib/guard/livereload/reactor/sockets.rb   | 4 ++++
 spec/lib/guard/livereload/reactor_spec.rb | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/guard/livereload/reactor.rb b/lib/guard/livereload/reactor.rb
index 5e3958c..d9ba970 100644
--- a/lib/guard/livereload/reactor.rb
+++ b/lib/guard/livereload/reactor.rb
@@ -49,7 +49,7 @@ def web_sockets
       # just to prevent semver change
       # (it's used only for tests anyway)
       def connections_count
-        sockets.internal_list.size
+        sockets.size
       end
 
       def stop
diff --git a/lib/guard/livereload/reactor/sockets.rb b/lib/guard/livereload/reactor/sockets.rb
index 15843e0..0a163d6 100644
--- a/lib/guard/livereload/reactor/sockets.rb
+++ b/lib/guard/livereload/reactor/sockets.rb
@@ -13,6 +13,10 @@ def internal_list
           @mutex.synchronize { @sockets }
         end
 
+        def size
+          @mutex.synchronize { @sockets.size }
+        end
+
         def broadcast(json)
           @mutex.synchronize do
             @sockets.each { |ws| ws.send(json) }
diff --git a/spec/lib/guard/livereload/reactor_spec.rb b/spec/lib/guard/livereload/reactor_spec.rb
index 9acd912..e4ede4b 100644
--- a/spec/lib/guard/livereload/reactor_spec.rb
+++ b/spec/lib/guard/livereload/reactor_spec.rb
@@ -92,7 +92,7 @@
         socket_list << socket
       end
 
-      allow(sockets).to receive(:internal_list).and_return(socket_list)
+      allow(sockets).to receive(:size) { socket_list.size }
     end
 
     context 'when connected once' do

From ac68bc1ae226aa85a94e3f37b885085d36d7b96a Mon Sep 17 00:00:00 2001
From: Cezary Baginski <cezary@chronomantic.net>
Date: Thu, 29 Oct 2015 23:37:33 +0100
Subject: [PATCH 7/7] show headers in debug mode

---
 lib/guard/livereload/websocket.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/guard/livereload/websocket.rb b/lib/guard/livereload/websocket.rb
index 38d1766..160b057 100644
--- a/lib/guard/livereload/websocket.rb
+++ b/lib/guard/livereload/websocket.rb
@@ -14,6 +14,8 @@ def initialize(options)
       def dispatch(data)
         parser = Http::Parser.new
         parser << data
+        UI.debug "Request: #{parser.request_url}"
+        UI.debug "Headers: #{parser.headers.inspect}"
         # prepend with '.' to make request url usable as a file path
         request_path = '.' + URI.parse(parser.request_url).path
         request_path += '/index.html' if File.directory? request_path