From 9ebceb889bbdedea25c496dd7c192027a96ff5cd Mon Sep 17 00:00:00 2001 From: Jeremy Thurgood Date: Sun, 3 Mar 2013 15:51:51 +0200 Subject: [PATCH 1/4] More comprehensive recursion guard. --- lib-topaz/array.rb | 43 +++++++++++---------- lib-topaz/bootstrap.rb | 1 + lib-topaz/thread.rb | 27 ++++++++++++++ tests/objects/test_threadobject.py | 60 ++++++++++++++++++++++++++++++ tests/test_executioncontext.py | 9 +++-- topaz/executioncontext.py | 25 +++++++++---- topaz/objects/fileobject.py | 5 ++- topaz/objects/threadobject.py | 18 +++++---- 8 files changed, 148 insertions(+), 40 deletions(-) create mode 100644 lib-topaz/thread.rb diff --git a/lib-topaz/array.rb b/lib-topaz/array.rb index cb8bb3710..d92ef5198 100644 --- a/lib-topaz/array.rb +++ b/lib-topaz/array.rb @@ -27,17 +27,18 @@ def initialize(size_or_arr = nil, obj = nil, &block) def inspect result = "[" - recursion = Thread.current.recursion_guard(self) do - self.each_with_index do |obj, i| - if i > 0 - result << ", " + Thread.current.recursion_guard(:array_inspect, self) do |recursion| + if recursion + result << "..." + else + self.each_with_index do |obj, i| + if i > 0 + result << ", " + end + result << obj.inspect end - result << obj.inspect end end - if recursion - result << "..." - end result << "]" end @@ -138,7 +139,8 @@ def first def flatten(level = -1) list = [] - recursion = Thread.current.recursion_guard(self) do + Thread.current.recursion_guard(:array_flatten, self) do |recursion| + raise ArgumentError.new("tried to flatten recursive array") if recursion self.each do |item| if level == 0 list << item @@ -150,9 +152,6 @@ def flatten(level = -1) end return list end - if recursion - raise ArgumentError.new("tried to flatten recursive array") - end end def flatten!(level = -1) @@ -178,10 +177,12 @@ def ==(other) if self.size != other.size return false end - Thread.current.recursion_guard(self) do - self.each_with_index do |x, i| - if x != other[i] - return false + Thread.current.recursion_guard(:array_equals, self) do |recursion| + if !recursion + self.each_with_index do |x, i| + if x != other[i] + return false + end end end end @@ -198,10 +199,12 @@ def eql?(other) if self.length != other.length return false end - Thread.current.recursion_guard(self) do - self.each_with_index do |x, i| - if !x.eql?(other[i]) - return false + Thread.current.recursion_guard(:array_eqlp, self) do |recursion| + if !recursion + self.each_with_index do |x, i| + if !x.eql?(other[i]) + return false + end end end end diff --git a/lib-topaz/bootstrap.rb b/lib-topaz/bootstrap.rb index 7601773d1..f38819736 100644 --- a/lib-topaz/bootstrap.rb +++ b/lib-topaz/bootstrap.rb @@ -5,6 +5,7 @@ load(File.join(lib_topaz, file)) end +load_bootstrap.call("thread.rb") load_bootstrap.call("array.rb") load_bootstrap.call("class.rb") load_bootstrap.call("comparable.rb") diff --git a/lib-topaz/thread.rb b/lib-topaz/thread.rb new file mode 100644 index 000000000..386aae3ed --- /dev/null +++ b/lib-topaz/thread.rb @@ -0,0 +1,27 @@ +class Thread + def recursion_guard_outer(identifier, obj, &block) + # We want to throw something less likely to be caught accidentally outside + # our own code than the recursion identifier. Ideally this should be an + # object that is unique to this particular recursion guard. Since doing + # that properly requires pushing extra state all the way up into + # ExecutionContext, we do this instead. + throw_symbol = "__recursion_guard_#{identifier}".to_sym + + if self.in_recursion_guard?(identifier) + self.recursion_guard(identifier, obj) do |recursion| + if !recursion + yield(false) + else + throw(throw_symbol) + end + end + else + self.recursion_guard(identifier, obj) do |recursion| + catch(throw_symbol) do + return yield(false) + end + return yield(true) + end + end + end +end diff --git a/tests/objects/test_threadobject.py b/tests/objects/test_threadobject.py index 158b61736..a40dde452 100644 --- a/tests/objects/test_threadobject.py +++ b/tests/objects/test_threadobject.py @@ -15,3 +15,63 @@ def test_thread_local_storage(self, space): return Thread.current[:a] """) assert space.int_w(w_res) == 1 + + def test_recursion_guard(self, space): + w_res = space.execute(""" + def foo(objs, depth = 0) + obj = objs.shift + Thread.current.recursion_guard(:foo, obj) do |recursion| + if recursion + [depth, obj] + else + foo(objs, depth + 1) + end + end + end + return foo([:a, :b, :c, :a, :d]) + """) + w_depth, w_symbol = space.listview(w_res) + assert space.int_w(w_depth) == 3 + assert space.symbol_w(w_symbol) == "a" + + def test_recursion_guard_nested(self, space): + w_res = space.execute(""" + def foo(objs, depth = 0) + obj = objs.shift + Thread.current.recursion_guard(:foo, obj) do |recursion| + return bar(objs, depth + 1) unless recursion + end + return [depth, obj] + end + + def bar(objs, depth) + obj = objs.shift + Thread.current.recursion_guard(:bar, obj) do |recursion| + return foo(objs, depth + 1) unless recursion + end + return [depth, obj] + end + + return foo([:a, :a, :b, :b, :c, :a, :d, :d]) + """) + w_depth, w_symbol = space.listview(w_res) + assert space.int_w(w_depth) == 5 + assert space.symbol_w(w_symbol) == "a" + + def test_recursion_guard_outer(self, space): + w_res = space.execute(""" + def foo(objs, depth = 0) + obj = objs.shift + Thread.current.recursion_guard_outer(:foo, obj) do |recursion| + if recursion + [depth, obj] + else + foo(objs, depth + 1) + end + end + end + return foo([:a, :b, :c, :a, :d]) + """) + w_depth, w_symbol = space.listview(w_res) + assert space.int_w(w_depth) == 0 + assert space.symbol_w(w_symbol) == "a" diff --git a/tests/test_executioncontext.py b/tests/test_executioncontext.py index e8d833262..60a400ffd 100644 --- a/tests/test_executioncontext.py +++ b/tests/test_executioncontext.py @@ -1,12 +1,13 @@ class TestExecutionContext(object): def test_recursion_guard(self, space): + f = object() x = object() y = object() - with space.getexecutioncontext().recursion_guard(x) as in_recursion: + with space.getexecutioncontext().recursion_guard(f, x) as in_recursion: assert not in_recursion - with space.getexecutioncontext().recursion_guard(y) as ir2: + with space.getexecutioncontext().recursion_guard(f, y) as ir2: assert not ir2 - with space.getexecutioncontext().recursion_guard(x) as ir3: + with space.getexecutioncontext().recursion_guard(f, x) as ir3: assert ir3 - with space.getexecutioncontext().recursion_guard(x) as ir3: + with space.getexecutioncontext().recursion_guard(f, x) as ir3: assert ir3 diff --git a/topaz/executioncontext.py b/topaz/executioncontext.py index b9799e248..a017a4d1c 100644 --- a/topaz/executioncontext.py +++ b/topaz/executioncontext.py @@ -13,7 +13,7 @@ def __init__(self): self.regexp_match_cell = None self.w_trace_proc = None self.in_trace_proc = False - self.recursive_objects = {} + self.recursive_calls = {} self.catch_names = {} def settraceproc(self, w_proc): @@ -75,8 +75,13 @@ def gettoprubyframe(self): frame = frame.backref() return frame - def recursion_guard(self, w_obj): - return _RecursionGuardContextManager(self, w_obj) + def recursion_guard(self, w_func_id, w_obj): + # We need independent recursion detection for different blocks of + # potentially recursive code so that they don't interfere with each + # other and cause false positives. This is only likely to be a problem + # if one recursion-guarded function calls another, but we can't + # guarantee that won't happen. + return _RecursionGuardContextManager(self, w_func_id, w_obj) def catch_block(self, name): return _CatchContextManager(self, name) @@ -103,21 +108,27 @@ def __exit__(self, exc_type, exc_value, tb): class _RecursionGuardContextManager(object): - def __init__(self, ec, w_obj): + def __init__(self, ec, w_func_id, w_obj): self.ec = ec + if w_func_id not in self.ec.recursive_calls: + self.ec.recursive_calls[w_func_id] = {} + self.recursive_objects = self.ec.recursive_calls[w_func_id] + self.w_func_id = w_func_id self.w_obj = w_obj self.added = False def __enter__(self): - if self.w_obj in self.ec.recursive_objects: + if self.w_obj in self.recursive_objects: return True - self.ec.recursive_objects[self.w_obj] = None + self.recursive_objects[self.w_obj] = None self.added = True return False def __exit__(self, exc_type, exc_value, tb): if self.added: - del self.ec.recursive_objects[self.w_obj] + del self.recursive_objects[self.w_obj] + if not self.recursive_objects: + del self.ec.recursive_calls[self.w_func_id] class _CatchContextManager(object): diff --git a/topaz/objects/fileobject.py b/topaz/objects/fileobject.py index 789268b7f..d04c677f3 100644 --- a/topaz/objects/fileobject.py +++ b/topaz/objects/fileobject.py @@ -386,11 +386,12 @@ def singleton_method_join(self, space, args_w): result = [] for w_arg in args_w: if isinstance(w_arg, W_ArrayObject): - with space.getexecutioncontext().recursion_guard(w_arg) as in_recursion: + ec = space.getexecutioncontext() + with ec.recursion_guard(space.newsymbol("file_singleton_method_join"), w_arg) as in_recursion: if in_recursion: raise space.error(space.w_ArgumentError, "recursive array") string = space.str_w( - W_FileObject.singleton_method_join(self, space, space.listview(w_arg)) + space.send(space.getclassfor(W_FileObject), space.newsymbol("join"), space.listview(w_arg)) ) else: w_string = space.convert_type(w_arg, space.w_string, "to_path", raise_error=False) diff --git a/topaz/objects/threadobject.py b/topaz/objects/threadobject.py index c3e7d7b12..15ba81ddc 100644 --- a/topaz/objects/threadobject.py +++ b/topaz/objects/threadobject.py @@ -31,13 +31,17 @@ def method_subscript_assign(self, space, key, w_value): return w_value @classdef.method("recursion_guard") - def method_recursion_guard(self, space, w_obj, block): + def method_recursion_guard(self, space, w_identifier, w_obj, block): """ - Detects recursion. If there is none, yield and return false. Else - return true + Calls the block with true if recursion is detected, false otherwise. + It is up to the block to decide what to do in either case. """ - with space.getexecutioncontext().recursion_guard(w_obj) as in_recursion: - if in_recursion: - return space.w_true - space.invoke_block(block, []) + ec = space.getexecutioncontext() + with ec.recursion_guard(w_identifier, w_obj) as in_recursion: + return space.invoke_block(block, [space.newbool(in_recursion)]) + + @classdef.method("in_recursion_guard?") + def method_in_recursion_guardp(self, space, w_identifier): + if w_identifier in space.getexecutioncontext().recursive_calls: + return space.w_true return space.w_false From 09c043b663de1e28de0816b9b21fd1f677395628 Mon Sep 17 00:00:00 2001 From: Jeremy Thurgood Date: Wed, 6 Mar 2013 18:46:21 +0200 Subject: [PATCH 2/4] Use rstrs instead of symbols for recursion identifiers. --- tests/test_executioncontext.py | 2 +- topaz/executioncontext.py | 16 ++++++++-------- topaz/objects/fileobject.py | 2 +- topaz/objects/threadobject.py | 7 +++++-- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/tests/test_executioncontext.py b/tests/test_executioncontext.py index 60a400ffd..474405b5d 100644 --- a/tests/test_executioncontext.py +++ b/tests/test_executioncontext.py @@ -1,6 +1,6 @@ class TestExecutionContext(object): def test_recursion_guard(self, space): - f = object() + f = "my_func" x = object() y = object() with space.getexecutioncontext().recursion_guard(f, x) as in_recursion: diff --git a/topaz/executioncontext.py b/topaz/executioncontext.py index a017a4d1c..94286e46e 100644 --- a/topaz/executioncontext.py +++ b/topaz/executioncontext.py @@ -75,13 +75,13 @@ def gettoprubyframe(self): frame = frame.backref() return frame - def recursion_guard(self, w_func_id, w_obj): + def recursion_guard(self, func_id, w_obj): # We need independent recursion detection for different blocks of # potentially recursive code so that they don't interfere with each # other and cause false positives. This is only likely to be a problem # if one recursion-guarded function calls another, but we can't # guarantee that won't happen. - return _RecursionGuardContextManager(self, w_func_id, w_obj) + return _RecursionGuardContextManager(self, func_id, w_obj) def catch_block(self, name): return _CatchContextManager(self, name) @@ -108,12 +108,12 @@ def __exit__(self, exc_type, exc_value, tb): class _RecursionGuardContextManager(object): - def __init__(self, ec, w_func_id, w_obj): + def __init__(self, ec, func_id, w_obj): self.ec = ec - if w_func_id not in self.ec.recursive_calls: - self.ec.recursive_calls[w_func_id] = {} - self.recursive_objects = self.ec.recursive_calls[w_func_id] - self.w_func_id = w_func_id + if func_id not in self.ec.recursive_calls: + self.ec.recursive_calls[func_id] = {} + self.recursive_objects = self.ec.recursive_calls[func_id] + self.func_id = func_id self.w_obj = w_obj self.added = False @@ -128,7 +128,7 @@ def __exit__(self, exc_type, exc_value, tb): if self.added: del self.recursive_objects[self.w_obj] if not self.recursive_objects: - del self.ec.recursive_calls[self.w_func_id] + del self.ec.recursive_calls[self.func_id] class _CatchContextManager(object): diff --git a/topaz/objects/fileobject.py b/topaz/objects/fileobject.py index d04c677f3..ab97bce5a 100644 --- a/topaz/objects/fileobject.py +++ b/topaz/objects/fileobject.py @@ -387,7 +387,7 @@ def singleton_method_join(self, space, args_w): for w_arg in args_w: if isinstance(w_arg, W_ArrayObject): ec = space.getexecutioncontext() - with ec.recursion_guard(space.newsymbol("file_singleton_method_join"), w_arg) as in_recursion: + with ec.recursion_guard("file_singleton_method_join", w_arg) as in_recursion: if in_recursion: raise space.error(space.w_ArgumentError, "recursive array") string = space.str_w( diff --git a/topaz/objects/threadobject.py b/topaz/objects/threadobject.py index 15ba81ddc..577b6cf7d 100644 --- a/topaz/objects/threadobject.py +++ b/topaz/objects/threadobject.py @@ -37,11 +37,14 @@ def method_recursion_guard(self, space, w_identifier, w_obj, block): It is up to the block to decide what to do in either case. """ ec = space.getexecutioncontext() - with ec.recursion_guard(w_identifier, w_obj) as in_recursion: + identifier = space.str_w(w_identifier) + with ec.recursion_guard(identifier, w_obj) as in_recursion: return space.invoke_block(block, [space.newbool(in_recursion)]) @classdef.method("in_recursion_guard?") def method_in_recursion_guardp(self, space, w_identifier): - if w_identifier in space.getexecutioncontext().recursive_calls: + ec = space.getexecutioncontext() + identifier = space.str_w(w_identifier) + if identifier in ec.recursive_calls: return space.w_true return space.w_false From 7baabb3910320204cca966dcb8607d27e65b9c1e Mon Sep 17 00:00:00 2001 From: Jeremy Thurgood Date: Thu, 7 Mar 2013 20:47:18 +0200 Subject: [PATCH 3/4] Switch back to previous Thread.recursion_guard() API. --- lib-topaz/array.rb | 41 +++++++++++++----------------- lib-topaz/thread.rb | 11 +++----- tests/objects/test_threadobject.py | 23 ++++++++--------- topaz/objects/threadobject.py | 4 ++- 4 files changed, 36 insertions(+), 43 deletions(-) diff --git a/lib-topaz/array.rb b/lib-topaz/array.rb index de6c38d74..3133b9a64 100644 --- a/lib-topaz/array.rb +++ b/lib-topaz/array.rb @@ -27,18 +27,17 @@ def initialize(size_or_arr = nil, obj = nil, &block) def inspect result = "[" - Thread.current.recursion_guard(:array_inspect, self) do |recursion| - if recursion - result << "..." - else - self.each_with_index do |obj, i| - if i > 0 - result << ", " - end - result << obj.inspect + recursion = Thread.current.recursion_guard(:array_inspect, self) do + self.each_with_index do |obj, i| + if i > 0 + result << ", " end + result << obj.inspect end end + if recursion + result << "..." + end result << "]" end @@ -139,8 +138,7 @@ def first def flatten(level = -1) list = [] - Thread.current.recursion_guard(:array_flatten, self) do |recursion| - raise ArgumentError.new("tried to flatten recursive array") if recursion + Thread.current.recursion_guard(:array_flatten, self) do self.each do |item| if level == 0 list << item @@ -152,6 +150,7 @@ def flatten(level = -1) end return list end + raise ArgumentError.new("tried to flatten recursive array") end def flatten!(level = -1) @@ -177,12 +176,10 @@ def ==(other) if self.size != other.size return false end - Thread.current.recursion_guard(:array_equals, self) do |recursion| - if !recursion - self.each_with_index do |x, i| - if x != other[i] - return false - end + Thread.current.recursion_guard(:array_equals, self) do + self.each_with_index do |x, i| + if x != other[i] + return false end end end @@ -199,12 +196,10 @@ def eql?(other) if self.length != other.length return false end - Thread.current.recursion_guard(:array_eqlp, self) do |recursion| - if !recursion - self.each_with_index do |x, i| - if !x.eql?(other[i]) - return false - end + Thread.current.recursion_guard(:array_eqlp, self) do + self.each_with_index do |x, i| + if !x.eql?(other[i]) + return false end end end diff --git a/lib-topaz/thread.rb b/lib-topaz/thread.rb index 386aae3ed..af5c9b0ff 100644 --- a/lib-topaz/thread.rb +++ b/lib-topaz/thread.rb @@ -8,15 +8,12 @@ def recursion_guard_outer(identifier, obj, &block) throw_symbol = "__recursion_guard_#{identifier}".to_sym if self.in_recursion_guard?(identifier) - self.recursion_guard(identifier, obj) do |recursion| - if !recursion - yield(false) - else - throw(throw_symbol) - end + self.recursion_guard(identifier, obj) do + return yield(false) end + throw(throw_symbol) else - self.recursion_guard(identifier, obj) do |recursion| + self.recursion_guard(identifier, obj) do catch(throw_symbol) do return yield(false) end diff --git a/tests/objects/test_threadobject.py b/tests/objects/test_threadobject.py index a40dde452..246d03081 100644 --- a/tests/objects/test_threadobject.py +++ b/tests/objects/test_threadobject.py @@ -20,12 +20,11 @@ def test_recursion_guard(self, space): w_res = space.execute(""" def foo(objs, depth = 0) obj = objs.shift - Thread.current.recursion_guard(:foo, obj) do |recursion| - if recursion - [depth, obj] - else - foo(objs, depth + 1) - end + recursion = Thread.current.recursion_guard(:foo, obj) do + return foo(objs, depth + 1) + end + if recursion + return [depth, obj] end end return foo([:a, :b, :c, :a, :d]) @@ -38,16 +37,16 @@ def test_recursion_guard_nested(self, space): w_res = space.execute(""" def foo(objs, depth = 0) obj = objs.shift - Thread.current.recursion_guard(:foo, obj) do |recursion| - return bar(objs, depth + 1) unless recursion + Thread.current.recursion_guard(:foo, obj) do + return bar(objs, depth + 1) end return [depth, obj] end def bar(objs, depth) obj = objs.shift - Thread.current.recursion_guard(:bar, obj) do |recursion| - return foo(objs, depth + 1) unless recursion + Thread.current.recursion_guard(:bar, obj) do + return foo(objs, depth + 1) end return [depth, obj] end @@ -64,9 +63,9 @@ def foo(objs, depth = 0) obj = objs.shift Thread.current.recursion_guard_outer(:foo, obj) do |recursion| if recursion - [depth, obj] + return [depth, obj] else - foo(objs, depth + 1) + return foo(objs, depth + 1) end end end diff --git a/topaz/objects/threadobject.py b/topaz/objects/threadobject.py index 577b6cf7d..acb1b4cbd 100644 --- a/topaz/objects/threadobject.py +++ b/topaz/objects/threadobject.py @@ -39,7 +39,9 @@ def method_recursion_guard(self, space, w_identifier, w_obj, block): ec = space.getexecutioncontext() identifier = space.str_w(w_identifier) with ec.recursion_guard(identifier, w_obj) as in_recursion: - return space.invoke_block(block, [space.newbool(in_recursion)]) + if not in_recursion: + space.invoke_block(block, []) + return space.newbool(in_recursion) @classdef.method("in_recursion_guard?") def method_in_recursion_guardp(self, space, w_identifier): From 2ad4cf24474bf1cb3091b3e56b01c3971645b5f6 Mon Sep 17 00:00:00 2001 From: Jeremy Thurgood Date: Thu, 7 Mar 2013 20:55:15 +0200 Subject: [PATCH 4/4] Switch API on Thread.recursion_guard_outer() as well. --- lib-topaz/thread.rb | 8 +++++--- tests/objects/test_threadobject.py | 9 +++------ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib-topaz/thread.rb b/lib-topaz/thread.rb index af5c9b0ff..81a99eefe 100644 --- a/lib-topaz/thread.rb +++ b/lib-topaz/thread.rb @@ -9,15 +9,17 @@ def recursion_guard_outer(identifier, obj, &block) if self.in_recursion_guard?(identifier) self.recursion_guard(identifier, obj) do - return yield(false) + yield + return false end throw(throw_symbol) else self.recursion_guard(identifier, obj) do catch(throw_symbol) do - return yield(false) + yield + return false end - return yield(true) + return true end end end diff --git a/tests/objects/test_threadobject.py b/tests/objects/test_threadobject.py index 246d03081..1179bb167 100644 --- a/tests/objects/test_threadobject.py +++ b/tests/objects/test_threadobject.py @@ -61,13 +61,10 @@ def test_recursion_guard_outer(self, space): w_res = space.execute(""" def foo(objs, depth = 0) obj = objs.shift - Thread.current.recursion_guard_outer(:foo, obj) do |recursion| - if recursion - return [depth, obj] - else - return foo(objs, depth + 1) - end + Thread.current.recursion_guard_outer(:foo, obj) do + return foo(objs, depth + 1) end + return [depth, obj] end return foo([:a, :b, :c, :a, :d]) """)