From 9643f10ea15eea26c42ce6b4c04268ef3ff75fd0 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Sun, 25 Dec 2022 20:20:05 +1300 Subject: [PATCH 1/4] List enumeration bug repro. --- lib/async/list.rb | 37 +++++++++++++++++++++++++++++++++++-- test/async/task.rb | 21 +++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/lib/async/list.rb b/lib/async/list.rb index 1cfb947e..f629eb5f 100644 --- a/lib/async/list.rb +++ b/lib/async/list.rb @@ -15,7 +15,7 @@ def initialize # Print a short summary of the list. def to_s - "#<#{self.class.name} size=#{@size}>" + sprintf("#<%s:0x%x size=%d>", self.class.name, object_id, @size) end alias inspect to_s @@ -121,6 +121,35 @@ def empty? @tail.equal?(self) end + private def validate!(node = nil) + previous = self + current = @tail + found = node.equal?(self) + + while true + break if current.equal?(self) + + if current.head != previous + raise "Invalid previous linked list node!" + end + + if current.is_a?(List) and !current.equal?(self) + raise "Invalid list in list node!" + end + + if node + found ||= current.equal?(node) + end + + previous = current + current = current.tail + end + + if node and !found + raise "Node not found in list!" + end + end + # Iterate over each node in the linked list. It is generally safe to remove the current node, any previous node or any future node during iteration. # # @yields {|node| ...} Yields each node in the list. @@ -130,9 +159,11 @@ def each current = self + $stderr.puts "-> each #{self}", caller while true + validate!(current) + node = current.tail - # binding.irb if node.nil? && !node.equal?(self) break if node.equal?(self) yield node @@ -144,6 +175,8 @@ def each end return self + ensure + $stderr.puts "<- each #{self}" end # Determine whether the given node is included in the list. diff --git a/test/async/task.rb b/test/async/task.rb index a98b44bd..94341dbe 100644 --- a/test/async/task.rb +++ b/test/async/task.rb @@ -414,6 +414,27 @@ expect(items).to be == [1, 2] end + + it "can stop a child task with transient children" do + parent = child = transient = nil + + reactor.run do |task| + parent = task.async do |task| + transient = task.async(transient: true) do + sleep(1) + end + + child = task.async do + sleep(1) + end + end + + parent.wait + parent.stop + expect(parent).to be(:complete?) + expect(transient).to be(:running?) + end.wait + end end with '#sleep' do From 9aa3f4ce40a1d4082ece21e1c2c5192c108ea688 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Mon, 26 Dec 2022 13:22:24 +1300 Subject: [PATCH 2/4] Use mutable (shift) list enumeration when reaping children. --- lib/async/list.rb | 9 ++++++--- lib/async/node.rb | 7 +++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/async/list.rb b/lib/async/list.rb index f629eb5f..48567423 100644 --- a/lib/async/list.rb +++ b/lib/async/list.rb @@ -159,7 +159,6 @@ def each current = self - $stderr.puts "-> each #{self}", caller while true validate!(current) @@ -175,8 +174,6 @@ def each end return self - ensure - $stderr.puts "<- each #{self}" end # Determine whether the given node is included in the list. @@ -204,6 +201,12 @@ def last @head end end + + def shift + if node = first + remove(node) + end + end end # A linked list Node. diff --git a/lib/async/node.rb b/lib/async/node.rb index fe1f1534..46618d13 100644 --- a/lib/async/node.rb +++ b/lib/async/node.rb @@ -187,13 +187,12 @@ def consume if parent = @parent and finished? parent.remove_child(self) + # If we have children, then we need to move them to our the parent if they are not finished: if @children - @children.each do |child| + while child = @children.shift if child.finished? - remove_child(child) + child.set_parent(nil) else - # In theory we don't need to do this... because we are throwing away the list. However, if you don't correctly update the list when moving the child to the parent, it foobars the enumeration, and subsequent nodes will be skipped, or in the worst case you might start enumerating the parents nodes. - remove_child(child) parent.add_child(child) end end From d2442ee7edea53c860349e98083f8859bda09d1e Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Mon, 26 Dec 2022 22:34:40 +1300 Subject: [PATCH 3/4] Minor comment update. --- lib/async/task.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/async/task.rb b/lib/async/task.rb index 4c774f24..776bf4e8 100644 --- a/lib/async/task.rb +++ b/lib/async/task.rb @@ -263,7 +263,7 @@ def schedule(&block) self.root.resume(@fiber) end - # Finish the current task, and all bound bound IO objects. + # Finish the current task, moving any children to the parent. def finish! # Allow the fiber to be recycled. @fiber = nil From b19ab450c40b0b5f3eff7e976fda4b027eb3ef27 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 28 Dec 2022 16:52:55 +1300 Subject: [PATCH 4/4] Robust list iteration using intrusive iterator. --- lib/async/list.rb | 151 ++++++++++++++++++++++++++++++++++++--------- test/async/task.rb | 3 +- 2 files changed, 124 insertions(+), 30 deletions(-) diff --git a/lib/async/list.rb b/lib/async/list.rb index 48567423..06ae19bb 100644 --- a/lib/async/list.rb +++ b/lib/async/list.rb @@ -20,6 +20,22 @@ def to_s alias inspect to_s + # Fast, safe, unbounded accumulation of children. + def to_a + items = [] + current = self + + while current.tail != self + unless current.tail.is_a?(Iterator) + items << current.tail + end + + current = current.tail + end + + return items + end + # Points at the end of the list. attr_accessor :head @@ -118,10 +134,10 @@ def remove(node) # @returns [Boolean] Returns true if the list is empty. def empty? - @tail.equal?(self) + @size == 0 end - private def validate!(node = nil) + def validate!(node = nil) previous = self current = @tail found = node.equal?(self) @@ -154,24 +170,10 @@ def empty? # # @yields {|node| ...} Yields each node in the list. # @returns [List] Returns self. - def each + def each(&block) return to_enum unless block_given? - current = self - - while true - validate!(current) - - node = current.tail - break if node.equal?(self) - - yield node - - # If the node has deleted itself or any subsequent node, it will no longer be the next node, so don't use it for continued traversal: - if current.tail.equal?(node) - current = node - end - end + Iterator.each(self, &block) return self end @@ -190,28 +192,119 @@ def include?(needle) # @returns [Node] Returns the first node in the list, if it is not empty. def first - unless @tail.equal?(self) - @tail + # validate! + + current = @tail + + while !current.equal?(self) + if current.is_a?(Iterator) + current = current.tail + else + return current + end end + + return nil end # @returns [Node] Returns the last node in the list, if it is not empty. def last - unless @head.equal?(self) - @head + # validate! + + current = @head + + while !current.equal?(self) + if current.is_a?(Iterator) + current = current.head + else + return current + end end + + return nil end def shift if node = first - remove(node) + remove!(node) end end - end - - # A linked list Node. - class List::Node - attr_accessor :head - attr_accessor :tail + + # A linked list Node. + class Node + attr_accessor :head + attr_accessor :tail + + alias inspect to_s + end + + class Iterator < Node + def initialize(list) + @list = list + + # Insert the iterator as the first item in the list: + @tail = list.tail + @tail.head = self + list.tail = self + @head = list + end + + def remove! + @head.tail = @tail + @tail.head = @head + @head = nil + @tail = nil + @list = nil + end + + def move_next + # Move to the next item (which could be an iterator or the end): + @tail.head = @head + @head.tail = @tail + @head = @tail + @tail = @tail.tail + @head.tail = self + @tail.head = self + end + + def move_current + while true + # Are we at the end of the list? + if @tail.equal?(@list) + return nil + end + + if @tail.is_a?(Iterator) + move_next + else + return @tail + end + end + end + + def each + while current = move_current + yield current + + if current.equal?(@tail) + move_next + end + end + end + + def self.each(list, &block) + list.validate! + + return if list.empty? + + iterator = Iterator.new(list) + + iterator.each(&block) + ensure + iterator&.remove! + end + end + + private_constant :Iterator end end diff --git a/test/async/task.rb b/test/async/task.rb index 94341dbe..e02e7b40 100644 --- a/test/async/task.rb +++ b/test/async/task.rb @@ -430,8 +430,9 @@ end parent.wait - parent.stop expect(parent).to be(:complete?) + parent.stop + expect(parent).to be(:stopped?) expect(transient).to be(:running?) end.wait end