diff --git a/cont.c b/cont.c index acfcefc81e89f2..8d5efeaac431a1 100644 --- a/cont.c +++ b/cont.c @@ -2201,7 +2201,7 @@ rb_fiber_storage_set(VALUE self, VALUE value) * Returns the value of the fiber storage variable identified by +key+. * * The +key+ must be a symbol, and the value is set by Fiber#[]= or - * Fiber#store. + * Fiber#storage. * * See also Fiber::[]=. */ diff --git a/ext/psych/lib/psych/versions.rb b/ext/psych/lib/psych/versions.rb index 2c2319e7a38e67..942495db9e894e 100644 --- a/ext/psych/lib/psych/versions.rb +++ b/ext/psych/lib/psych/versions.rb @@ -5,6 +5,6 @@ module Psych VERSION = '5.3.0' if RUBY_ENGINE == 'jruby' - DEFAULT_SNAKEYAML_VERSION = '2.9'.freeze + DEFAULT_SNAKEYAML_VERSION = '2.10'.freeze end end diff --git a/gc.c b/gc.c index 5f0f2307c8c9fd..8b6142b139650e 100644 --- a/gc.c +++ b/gc.c @@ -2061,6 +2061,7 @@ obj_free_object_id(VALUE obj) void rb_gc_obj_free_vm_weak_references(VALUE obj) { + ASSUME(!RB_SPECIAL_CONST_P(obj)); obj_free_object_id(obj); if (rb_obj_gen_fields_p(obj)) { diff --git a/internal/class.h b/internal/class.h index 296a07ae29e9f3..ea68b07fc20968 100644 --- a/internal/class.h +++ b/internal/class.h @@ -658,8 +658,8 @@ RCLASS_SET_REFINED_CLASS(VALUE klass, VALUE refined) static inline rb_alloc_func_t RCLASS_ALLOCATOR(VALUE klass) { - RUBY_ASSERT(RB_TYPE_P(klass, T_CLASS) || RB_TYPE_P(klass, T_ICLASS)); - if (RCLASS_SINGLETON_P(klass) || RB_TYPE_P(klass, T_ICLASS)) { + RBIMPL_ASSERT_TYPE(klass, T_CLASS); + if (RCLASS_SINGLETON_P(klass)) { return 0; } return RCLASS_EXT_PRIME(klass)->as.class.allocator; diff --git a/lib/timeout.rb b/lib/timeout.rb index 1640542d6f806a..9969fa2e57b5d7 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -22,7 +22,7 @@ module Timeout # The version VERSION = "0.5.0" - # Internal error raised to when a timeout is triggered. + # Internal exception raised to when a timeout is triggered. class ExitException < Exception def exception(*) # :nodoc: self @@ -54,8 +54,6 @@ def self.handle_timeout(message) # :nodoc: private_constant :GET_TIME class State - attr_reader :condvar, :queue, :queue_mutex # shared with Timeout.timeout() - def initialize @condvar = ConditionVariable.new @queue = Queue.new @@ -83,36 +81,40 @@ def self.instance end def create_timeout_thread - watcher = Thread.new do - requests = [] - while true - until @queue.empty? and !requests.empty? # wait to have at least one request - req = @queue.pop - requests << req unless req.done? - end - closest_deadline = requests.min_by(&:deadline).deadline + # Threads unexpectedly inherit the interrupt mask: https://github.com/ruby/timeout/issues/41 + # So reset the interrupt mask to the default one for the timeout thread + Thread.handle_interrupt(Object => :immediate) do + watcher = Thread.new do + requests = [] + while true + until @queue.empty? and !requests.empty? # wait to have at least one request + req = @queue.pop + requests << req unless req.done? + end + closest_deadline = requests.min_by(&:deadline).deadline - now = 0.0 - @queue_mutex.synchronize do - while (now = GET_TIME.call(Process::CLOCK_MONOTONIC)) < closest_deadline and @queue.empty? - @condvar.wait(@queue_mutex, closest_deadline - now) + now = 0.0 + @queue_mutex.synchronize do + while (now = GET_TIME.call(Process::CLOCK_MONOTONIC)) < closest_deadline and @queue.empty? + @condvar.wait(@queue_mutex, closest_deadline - now) + end end - end - requests.each do |req| - req.interrupt if req.expired?(now) + requests.each do |req| + req.interrupt if req.expired?(now) + end + requests.reject!(&:done?) end - requests.reject!(&:done?) end - end - if !watcher.group.enclosed? && (!defined?(Ractor.main?) || Ractor.main?) - ThreadGroup::Default.add(watcher) - end + if !watcher.group.enclosed? && (!defined?(Ractor.main?) || Ractor.main?) + ThreadGroup::Default.add(watcher) + end - watcher.name = "Timeout stdlib thread" - watcher.thread_variable_set(:"\0__detached_thread__", true) - watcher + watcher.name = "Timeout stdlib thread" + watcher.thread_variable_set(:"\0__detached_thread__", true) + watcher + end end def ensure_timeout_thread_created @@ -121,13 +123,20 @@ def ensure_timeout_thread_created # In that case, just return and let the main thread create the Timeout thread. return if @timeout_thread_mutex.owned? - @timeout_thread_mutex.synchronize do + Sync.synchronize @timeout_thread_mutex do unless @timeout_thread&.alive? @timeout_thread = create_timeout_thread end end end end + + def add_request(request) + Sync.synchronize @queue_mutex do + @queue << request + @condvar.signal + end + end end private_constant :State @@ -144,6 +153,7 @@ def initialize(thread, timeout, exception_class, message) @done = false # protected by @mutex end + # Only called by the timeout thread, so does not need Sync.synchronize def done? @mutex.synchronize do @done @@ -154,6 +164,7 @@ def expired?(now) now >= @deadline end + # Only called by the timeout thread, so does not need Sync.synchronize def interrupt @mutex.synchronize do unless @done @@ -164,16 +175,36 @@ def interrupt end def finished - @mutex.synchronize do + Sync.synchronize @mutex do @done = true end end end private_constant :Request + module Sync + # Calls mutex.synchronize(&block) but if that fails on CRuby due to being in a trap handler, + # run mutex.synchronize(&block) in a separate Thread instead. + def self.synchronize(mutex, &block) + begin + mutex.synchronize(&block) + rescue ThreadError => e + raise e unless e.message == "can't be called from trap context" + # Workaround CRuby issue https://bugs.ruby-lang.org/issues/19473 + # which raises on Mutex#synchronize in trap handler. + # It's expensive to create a Thread just for this, + # but better than failing. + Thread.new { + mutex.synchronize(&block) + }.join + end + end + end + private_constant :Sync + # :startdoc: - # Perform an operation in a block, raising an error if it takes longer than + # Perform an operation in a block, raising an exception if it takes longer than # +sec+ seconds to complete. # # +sec+:: Number of seconds to wait for the block to terminate. Any non-negative number @@ -186,12 +217,18 @@ def finished # Omitting will use the default, "execution expired" # # Returns the result of the block *if* the block completed before - # +sec+ seconds, otherwise throws an exception, based on the value of +klass+. + # +sec+ seconds, otherwise raises an exception, based on the value of +klass+. # - # The exception thrown to terminate the given block cannot be rescued inside - # the block unless +klass+ is given explicitly. However, the block can use - # ensure to prevent the handling of the exception. For that reason, this - # method cannot be relied on to enforce timeouts for untrusted blocks. + # The exception raised to terminate the given block is the given +klass+, or + # Timeout::ExitException if +klass+ is not given. The reason for that behavior + # is that Timeout::Error inherits from RuntimeError and might be caught unexpectedly by `rescue`. + # Timeout::ExitException inherits from Exception so it will only be rescued by `rescue Exception`. + # Note that the Timeout::ExitException is translated to a Timeout::Error once it reaches the Timeout.timeout call, + # so outside that call it will be a Timeout::Error. + # + # In general, be aware that the code block may rescue the exception, and in such a case not respect the timeout. + # Also, the block can use +ensure+ to prevent the handling of the exception. + # For those reasons, this method cannot be relied on to enforce timeouts for untrusted blocks. # # If a scheduler is defined, it will be used to handle the timeout by invoking # Scheduler#timeout_after. @@ -199,6 +236,45 @@ def finished # Note that this is both a method of module Timeout, so you can include # Timeout into your classes so they have a #timeout method, as well as # a module method, so you can call it directly as Timeout.timeout(). + # + # ==== Ensuring the exception does not fire inside ensure blocks + # + # When using Timeout.timeout it can be desirable to ensure the timeout exception does not fire inside an +ensure+ block. + # The simplest and best way to do so it to put the Timeout.timeout call inside the body of the begin/ensure/end: + # + # begin + # Timeout.timeout(sec) { some_long_operation } + # ensure + # cleanup # safe, cannot be interrupt by timeout + # end + # + # If that is not feasible, e.g. if there are +ensure+ blocks inside +some_long_operation+, + # they need to not be interrupted by timeout, and it's not possible to move these ensure blocks outside, + # one can use Thread.handle_interrupt to delay the timeout exception like so: + # + # Thread.handle_interrupt(Timeout::Error => :never) { + # Timeout.timeout(sec, Timeout::Error) do + # setup # timeout cannot happen here, no matter how long it takes + # Thread.handle_interrupt(Timeout::Error => :immediate) { + # some_long_operation # timeout can happen here + # } + # ensure + # cleanup # timeout cannot happen here, no matter how long it takes + # end + # } + # + # An important thing to note is the need to pass an exception klass to Timeout.timeout, + # otherwise it does not work. Specifically, using +Thread.handle_interrupt(Timeout::ExitException => ...)+ + # is unsupported and causes subtle errors like raising the wrong exception outside the block, do not use that. + # + # Note that Thread.handle_interrupt is somewhat dangerous because if setup or cleanup hangs + # then the current thread will hang too and the timeout will never fire. + # Also note the block might run for longer than +sec+ seconds: + # e.g. some_long_operation executes for +sec+ seconds + whatever time cleanup takes. + # + # If you want the timeout to only happen on blocking operations one can use :on_blocking + # instead of :immediate. However, that means if the block uses no blocking operations after +sec+ seconds, + # the block will not be interrupted. def self.timeout(sec, klass = nil, message = nil, &block) #:yield: +sec+ return yield(sec) if sec == nil or sec.zero? raise ArgumentError, "Timeout sec must be a non-negative number" if 0 > sec @@ -214,10 +290,7 @@ def self.timeout(sec, klass = nil, message = nil, &block) #:yield: +sec+ perform = Proc.new do |exc| request = Request.new(Thread.current, sec, exc, message) - state.queue_mutex.synchronize do - state.queue << request - state.condvar.signal - end + state.add_request(request) begin return yield(sec) ensure diff --git a/object.c b/object.c index bcafab3c3d4bf0..158bb0b219256c 100644 --- a/object.c +++ b/object.c @@ -2238,8 +2238,10 @@ class_call_alloc_func(rb_alloc_func_t allocator, VALUE klass) obj = (*allocator)(klass); - if (rb_obj_class(obj) != rb_class_real(klass)) { - rb_raise(rb_eTypeError, "wrong instance allocation"); + if (UNLIKELY(RBASIC_CLASS(obj) != klass)) { + if (rb_obj_class(obj) != rb_class_real(klass)) { + rb_raise(rb_eTypeError, "wrong instance allocation"); + } } return obj; } diff --git a/set.c b/set.c index 7ef7c11d45de56..0c657cf66d8118 100644 --- a/set.c +++ b/set.c @@ -1291,15 +1291,17 @@ set_xor_i(st_data_t key, st_data_t data) static VALUE set_i_xor(VALUE set, VALUE other) { - VALUE new_set; + VALUE new_set = rb_obj_dup(set); + if (rb_obj_is_kind_of(other, rb_cSet)) { - new_set = other; + set_iter(other, set_xor_i, (st_data_t)new_set); } else { - new_set = set_s_alloc(rb_obj_class(set)); - set_merge_enum_into(new_set, other); + VALUE tmp = set_s_alloc(rb_cSet); + set_merge_enum_into(tmp, other); + set_iter(tmp, set_xor_i, (st_data_t)new_set); } - set_iter(set, set_xor_i, (st_data_t)new_set); + return new_set; } diff --git a/test/json/json_parser_test.rb b/test/json/json_parser_test.rb index d29f8077b1d138..ec9391909d779d 100644 --- a/test/json/json_parser_test.rb +++ b/test/json/json_parser_test.rb @@ -843,7 +843,6 @@ def test_parse_whitespace_after_newline def test_frozen parser_config = JSON::Parser::Config.new({}).freeze - omit "JRuby failure in CI" if RUBY_ENGINE == "jruby" assert_raise FrozenError do parser_config.send(:initialize, {}) end diff --git a/test/ruby/test_set.rb b/test/ruby/test_set.rb index 6dec0d41ae9a48..e8ac3e329e6678 100644 --- a/test/ruby/test_set.rb +++ b/test/ruby/test_set.rb @@ -703,6 +703,17 @@ def test_xor } end + def test_xor_does_not_mutate_other_set + a = Set[1] + b = Set[1, 2] + original_b = b.dup + + result = a ^ b + + assert_equal(original_b, b) + assert_equal(Set[2], result) + end + def test_eq set1 = Set[2,3,1] set2 = Set[1,2,3] diff --git a/test/test_timeout.rb b/test/test_timeout.rb index 51666b73d8e52a..7421b5ba4174c8 100644 --- a/test/test_timeout.rb +++ b/test/test_timeout.rb @@ -4,6 +4,12 @@ class TestTimeout < Test::Unit::TestCase + private def kill_timeout_thread + thread = Timeout.const_get(:State).instance.instance_variable_get(:@timeout_thread) + thread.kill + thread.join + end + def test_public_methods assert_equal [:timeout], Timeout.private_instance_methods(false) assert_equal [], Timeout.public_instance_methods(false) @@ -221,6 +227,24 @@ def o.each end end + def test_handle_interrupt_with_exception_class + bug11344 = '[ruby-dev:49179] [Bug #11344]' + ok = false + assert_raise(Timeout::Error) { + Thread.handle_interrupt(Timeout::Error => :never) { + Timeout.timeout(0.01, Timeout::Error) { + sleep 0.2 + ok = true + Thread.handle_interrupt(Timeout::Error => :on_blocking) { + sleep 0.2 + raise "unreachable" + } + } + } + } + assert(ok, bug11344) + end + def test_handle_interrupt bug11344 = '[ruby-dev:49179] [Bug #11344]' ok = false @@ -231,6 +255,7 @@ def test_handle_interrupt ok = true Thread.handle_interrupt(Timeout::ExitException => :on_blocking) { sleep 0.2 + raise "unreachable" } } } @@ -238,6 +263,94 @@ def test_handle_interrupt assert(ok, bug11344) end + def test_handle_interrupt_with_interrupt_mask_inheritance + issue = 'https://github.com/ruby/timeout/issues/41' + + [ + -> {}, # not blocking so no opportunity to interrupt + -> { sleep 5 } + ].each_with_index do |body, idx| + # We need to create a new Timeout thread + kill_timeout_thread + + # Create the timeout thread under a handle_interrupt(:never) + # due to the interrupt mask being inherited + Thread.handle_interrupt(Object => :never) { + assert_equal :ok, Timeout.timeout(1) { :ok } + } + + # Ensure a simple timeout works and the interrupt mask was not inherited + assert_raise(Timeout::Error) { + Timeout.timeout(0.001) { sleep 1 } + } + + r = [] + # This raises Timeout::ExitException and not Timeout::Error for the non-blocking body + # because of the handle_interrupt(:never) which delays raising Timeout::ExitException + # on the main thread until getting outside of that handle_interrupt(:never) call. + # For this reason we document handle_interrupt(Timeout::ExitException) should not be used. + exc = idx == 0 ? Timeout::ExitException : Timeout::Error + assert_raise(exc) { + Thread.handle_interrupt(Timeout::ExitException => :never) { + Timeout.timeout(0.1) do + sleep 0.2 + r << :sleep_before_done + Thread.handle_interrupt(Timeout::ExitException => :on_blocking) { + r << :body + body.call + } + ensure + sleep 0.2 + r << :ensure_sleep_done + end + } + } + assert_equal([:sleep_before_done, :body, :ensure_sleep_done], r, issue) + end + end + + # Same as above but with an exception class + def test_handle_interrupt_with_interrupt_mask_inheritance_with_exception_class + issue = 'https://github.com/ruby/timeout/issues/41' + + [ + -> {}, # not blocking so no opportunity to interrupt + -> { sleep 5 } + ].each do |body| + # We need to create a new Timeout thread + kill_timeout_thread + + # Create the timeout thread under a handle_interrupt(:never) + # due to the interrupt mask being inherited + Thread.handle_interrupt(Object => :never) { + assert_equal :ok, Timeout.timeout(1) { :ok } + } + + # Ensure a simple timeout works and the interrupt mask was not inherited + assert_raise(Timeout::Error) { + Timeout.timeout(0.001) { sleep 1 } + } + + r = [] + assert_raise(Timeout::Error) { + Thread.handle_interrupt(Timeout::Error => :never) { + Timeout.timeout(0.1, Timeout::Error) do + sleep 0.2 + r << :sleep_before_done + Thread.handle_interrupt(Timeout::Error => :on_blocking) { + r << :body + body.call + } + ensure + sleep 0.2 + r << :ensure_sleep_done + end + } + } + assert_equal([:sleep_before_done, :body, :ensure_sleep_done], r, issue) + end + end + def test_fork omit 'fork not supported' unless Process.respond_to?(:fork) r, w = IO.pipe @@ -303,4 +416,33 @@ def test_ractor assert_equal :ok, r end; end if defined?(::Ractor) && RUBY_VERSION >= '4.0' + + def test_timeout_in_trap_handler + # https://github.com/ruby/timeout/issues/17 + + # Test as if this was the first timeout usage + kill_timeout_thread + + rd, wr = IO.pipe + + trap("SIGUSR1") do + begin + Timeout.timeout(0.1) do + sleep 1 + end + rescue Timeout::Error + wr.write "OK" + wr.close + else + wr.write "did not raise" + ensure + wr.close + end + end + + Process.kill :USR1, Process.pid + + assert_equal "OK", rd.read + rd.close + end end diff --git a/vm_method.c b/vm_method.c index dbc5ad97eded92..17f68fc258ad16 100644 --- a/vm_method.c +++ b/vm_method.c @@ -1635,10 +1635,20 @@ rb_undef_alloc_func(VALUE klass) rb_alloc_func_t rb_get_alloc_func(VALUE klass) { - Check_Type(klass, T_CLASS); + RBIMPL_ASSERT_TYPE(klass, T_CLASS); - for (; klass; klass = RCLASS_SUPER(klass)) { - rb_alloc_func_t allocator = RCLASS_ALLOCATOR(klass); + rb_alloc_func_t allocator = RCLASS_ALLOCATOR(klass); + if (allocator == UNDEF_ALLOC_FUNC) return 0; + if (allocator) return allocator; + + VALUE *superclasses = RCLASS_SUPERCLASSES(klass); + size_t depth = RCLASS_SUPERCLASS_DEPTH(klass); + + for (size_t i = depth; i > 0; i--) { + klass = superclasses[i - 1]; + RBIMPL_ASSERT_TYPE(klass, T_CLASS); + + allocator = RCLASS_ALLOCATOR(klass); if (allocator == UNDEF_ALLOC_FUNC) break; if (allocator) return allocator; }