Skip to content
This repository
Browse code

Fix a fairly bad bug in event de-duplication

This is fairly edge-case-y but could bite someone. If you'd set a watch
when doing a get that failed because the node didn't exist, any subsequent
attempts to set a watch would fail silently, because the client thought that the
watch had already been set.

We now wrap the operation in the setup_watcher! method, which rolls back the
record-keeping of what watches have already been set for what nodes if an
exception is raised.

This change has the side-effect that certain operations (get,stat,exists?,children)
will block event delivery until completion, because they need to have a consistent
idea about what events are pending, and which have been delivered. This also means
that calling these methods represent a synchronization point between user threads
(these operations can only occur serially, not simultaneously).
  • Loading branch information...
commit 9ca2f901d494ab326d3d842240ed9663de644547 1 parent 8fa292c
Jonathan Simms authored April 26, 2012
33  lib/z_k/client/base.rb
@@ -326,9 +326,9 @@ def create(path, data='', opts={})
326 326
       def get(path, opts={})
327 327
         h = { :path => path }.merge(opts)
328 328
 
329  
-        setup_watcher!(:data, h)
330  
-
331  
-        rv = check_rc(cnx.get(h), h)
  329
+        rv = setup_watcher!(:data, h) do
  330
+          check_rc(cnx.get(h), h)
  331
+        end
332 332
 
333 333
         opts[:callback] ? rv : rv.values_at(:data, :stat)
334 334
       end
@@ -454,17 +454,17 @@ def stat(path, opts={})
454 454
 
455 455
         h = { :path => path }.merge(opts)
456 456
 
457  
-        setup_watcher!(:data, h)
  457
+        setup_watcher!(:data, h) do
  458
+          rv = cnx.stat(h)
458 459
 
459  
-        rv = cnx.stat(h)
  460
+          return rv if opts[:callback] 
460 461
 
461  
-        return rv if opts[:callback] 
462  
-
463  
-        case rv[:rc] 
464  
-        when Zookeeper::ZOK, Zookeeper::ZNONODE
465  
-          rv[:stat]
466  
-        else
467  
-          check_rc(rv, h) # throws the appropriate error
  462
+          case rv[:rc] 
  463
+          when Zookeeper::ZOK, Zookeeper::ZNONODE
  464
+            rv[:stat]
  465
+          else
  466
+            check_rc(rv, h) # throws the appropriate error
  467
+          end
468 468
         end
469 469
       end
470 470
 
@@ -548,9 +548,10 @@ def children(path, opts={})
548 548
 
549 549
         h = { :path => path }.merge(opts)
550 550
 
551  
-        setup_watcher!(:child, h)
  551
+        rv = setup_watcher!(:child, h) do
  552
+          check_rc(cnx.get_children(h), h)
  553
+        end
552 554
 
553  
-        rv = check_rc(cnx.get_children(h), h)
554 555
         opts[:callback] ? rv : rv[:children]
555 556
       end
556 557
 
@@ -798,8 +799,8 @@ def check_rc(hash, inputs=nil)
798 799
         end
799 800
 
800 801
         # @private
801  
-        def setup_watcher!(watch_type, opts)
802  
-          event_handler.setup_watcher!(watch_type, opts)
  802
+        def setup_watcher!(watch_type, opts, &b)
  803
+          event_handler.setup_watcher!(watch_type, opts, &b)
803 804
         end
804 805
 
805 806
         # used in #inspect, doesn't raise an error if we're not connected
36  lib/z_k/event_handler.rb
@@ -153,26 +153,52 @@ def get_default_watcher_block
153 153
       end
154 154
     end
155 155
 
  156
+    # returns true if there's a pending watch of type for path
  157
+    # @private
  158
+    def restricting_new_watches_for?(watch_type, path)
  159
+      synchronize do
  160
+        if set = @outstanding_watches[watch_type]
  161
+          return set.include?(path)
  162
+        end
  163
+      end
  164
+
  165
+      false
  166
+    end
  167
+
156 168
     # implements not only setting up the watcher callback, but deduplicating 
157 169
     # event delivery. Keeps track of in-flight watcher-type+path requests and
158 170
     # doesn't re-register the watcher with the server until a response has been
159 171
     # fired. This prevents one event delivery to *every* callback per :watch => true
160 172
     # argument.
161 173
     #
  174
+    # due to somewhat poor design, we destructively modify opts before we yield
  175
+    # and the client implictly knows this
  176
+    #
162 177
     # @private
163 178
     def setup_watcher!(watch_type, opts)
164  
-      return unless opts.delete(:watch)
  179
+      return yield unless opts.delete(:watch)
165 180
 
166 181
       synchronize do
167 182
         set = @outstanding_watches.fetch(watch_type)
168 183
         path = opts[:path]
169 184
 
170 185
         if set.add?(path)
171  
-          # this path has no outstanding watchers, let it do its thing
172  
-          opts[:watcher] = watcher_callback 
  186
+          # if we added the path to the set, blocking further registration of
  187
+          # watches and an exception is raised then we rollback
  188
+          begin
  189
+            # this path has no outstanding watchers, let it do its thing
  190
+            opts[:watcher] = watcher_callback 
  191
+
  192
+            yield opts
  193
+          rescue Exception
  194
+            set.delete(path)
  195
+            raise
  196
+          end
173 197
         else
174  
-          # outstanding watch for path and data pair already exists, so ignore
175  
-#           logger.debug { "outstanding watch request for path #{path.inspect} and watcher type #{watch_type.inspect}, not re-registering" }
  198
+          # we did not add the path to the set, which means we are not
  199
+          # responsible for removing a block on further adds if the operation
  200
+          # fails, therefore, we just yield
  201
+          yield opts
176 202
         end
177 203
       end
178 204
     end
61  spec/watch_spec.rb
... ...
@@ -1,4 +1,4 @@
1  
-require File.join(File.dirname(__FILE__), %w[spec_helper])
  1
+require 'spec_helper'
2 2
 
3 3
 describe ZK do
4 4
   describe do
@@ -8,15 +8,18 @@
8 8
 
9 9
       @path = "/_testWatch"
10 10
       wait_until { @zk.connected? }
  11
+
  12
+      # make sure we start w/ clean state
  13
+      @zk.rm_rf(@path)
11 14
     end
12 15
 
13 16
     after do
14  
-      if @zk.connected?
15  
-        @zk.close! 
16  
-        wait_until { !@zk.connected? }
17  
-      end
18  
-
19 17
       mute_logger do
  18
+        if @zk.connected?
  19
+          @zk.close! 
  20
+          wait_until { !@zk.connected? }
  21
+        end
  22
+
20 23
         ZK.open(@cnx_str) { |zk| zk.rm_rf(@path) }
21 24
       end
22 25
     end
@@ -25,7 +28,7 @@
25 28
       locker = Mutex.new
26 29
       callback_called = false
27 30
 
28  
-      @zk.watcher.register(@path) do |event|
  31
+      @zk.register(@path) do |event|
29 32
         locker.synchronize do
30 33
           callback_called = true
31 34
         end
@@ -52,7 +55,7 @@ def wait_for_events_to_not_be_delivered(events)
52 55
     it %[should only deliver an event once to each watcher registered for exists?] do
53 56
       events = []
54 57
 
55  
-      sub = @zk.watcher.register(@path) do |ev|
  58
+      sub = @zk.register(@path) do |ev|
56 59
         logger.debug "got event #{ev}"
57 60
         events << ev
58 61
       end
@@ -73,7 +76,7 @@ def wait_for_events_to_not_be_delivered(events)
73 76
 
74 77
       @zk.create(@path, 'one', :mode => :ephemeral)
75 78
 
76  
-      sub = @zk.watcher.register(@path) do |ev|
  79
+      sub = @zk.register(@path) do |ev|
77 80
         logger.debug "got event #{ev}"
78 81
         events << ev
79 82
       end
@@ -96,7 +99,7 @@ def wait_for_events_to_not_be_delivered(events)
96 99
 
97 100
       @zk.create(@path, '')
98 101
 
99  
-      sub = @zk.watcher.register(@path) do |ev|
  102
+      sub = @zk.register(@path) do |ev|
100 103
         logger.debug "got event #{ev}"
101 104
         events << ev
102 105
       end
@@ -112,6 +115,40 @@ def wait_for_events_to_not_be_delivered(events)
112 115
 
113 116
       events.length.should == 1
114 117
     end
  118
+
  119
+    it %[should restrict_new_watches_for? if a successul watch has been set] do
  120
+      @zk.stat(@path, watch: true)
  121
+      @zk.event_handler.should be_restricting_new_watches_for(:data, @path)
  122
+    end
  123
+
  124
+    it %[should not a block on new watches after an operation fails] do
  125
+      # this is a situation where we did get('/blah', :watch => true) but
  126
+      # got an exception, the next watch set should work
  127
+
  128
+      events = []
  129
+
  130
+      sub = @zk.register(@path) do |ev|
  131
+        logger.debug { "got event #{ev}" }
  132
+        events << ev
  133
+      end
  134
+
  135
+      # get a path that doesn't exist with a watch
  136
+
  137
+      lambda { @zk.get(@path, watch: true) }.should raise_error(ZK::Exceptions::NoNode)
  138
+
  139
+      @zk.event_handler.should_not be_restricting_new_watches_for(:data, @path)
  140
+
  141
+      @zk.stat(@path, watch: true)
  142
+
  143
+      @zk.event_handler.should be_restricting_new_watches_for(:data, @path)
  144
+
  145
+      @zk.create(@path, '')
  146
+
  147
+      wait_while { events.empty? }
  148
+
  149
+      events.should_not be_empty
  150
+
  151
+    end
115 152
   end
116 153
 
117 154
   describe 'state watcher' do
@@ -141,10 +178,6 @@ def wait_for_events_to_not_be_delivered(events)
141 178
           m.should_receive(:state).and_return(ZookeeperConstants::ZOO_CONNECTED_STATE)
142 179
         end
143 180
       end
144  
-
145  
-      it %[should only fire the callback once] do
146  
-        pending "not sure if this is the behavior we want"
147  
-      end
148 181
     end
149 182
   end
150 183
 end

0 notes on commit 9ca2f90

Please sign in to comment.
Something went wrong with that request. Please try again.