ruby-changes:74059
From: Jean <ko1@a...>
Date: Mon, 17 Oct 2022 23:56:24 +0900 (JST)
Subject: [ruby-changes:74059] 60defe0a68 (master): thread_sync.c: Clarify and document the behavior of timeout == 0
https://git.ruby-lang.org/ruby.git/commit/?id=60defe0a68 From 60defe0a68a40d1b3225cf6b971ea195e19ae2e2 Mon Sep 17 00:00:00 2001 From: Jean Boussier <jean.boussier@g...> Date: Thu, 6 Oct 2022 15:53:16 +0200 Subject: thread_sync.c: Clarify and document the behavior of timeout == 0 [Feature #18982] Instead of introducing an `exception: false` argument to have `non_block` return nil rather than raise, we can clearly document that a timeout of 0 immediately returns. The code is refactored a bit to avoid doing a time calculation in such case. --- spec/ruby/shared/queue/deque.rb | 7 ++++++ spec/ruby/shared/sizedqueue/enque.rb | 8 ++++++- thread_sync.c | 45 ++++++++++++++++++++---------------- thread_sync.rb | 6 ++--- 4 files changed, 42 insertions(+), 24 deletions(-) diff --git a/spec/ruby/shared/queue/deque.rb b/spec/ruby/shared/queue/deque.rb index ed32bd29c8..616e56ec8a 100644 --- a/spec/ruby/shared/queue/deque.rb +++ b/spec/ruby/shared/queue/deque.rb @@ -87,6 +87,13 @@ describe :queue_deq, shared: true do https://github.com/ruby/ruby/blob/trunk/spec/ruby/shared/queue/deque.rb#L87 t.join end + it "immediately returns nil if no item is available and the timeout is 0" do + q = @object.call + q << 1 + q.send(@method, timeout: 0).should == 1 + q.send(@method, timeout: 0).should == nil + end + it "raise TypeError if timeout is not a valid numeric" do q = @object.call -> { q.send(@method, timeout: "1") }.should raise_error( diff --git a/spec/ruby/shared/sizedqueue/enque.rb b/spec/ruby/shared/sizedqueue/enque.rb index 126470594a..059f1025a7 100644 --- a/spec/ruby/shared/sizedqueue/enque.rb +++ b/spec/ruby/shared/sizedqueue/enque.rb @@ -73,7 +73,13 @@ describe :sizedqueue_enq, shared: true do https://github.com/ruby/ruby/blob/trunk/spec/ruby/shared/sizedqueue/enque.rb#L73 t.join end - it "returns nil if no item is available in time" do + it "returns nil if no space is avialable and timeout is 0" do + q = @object.call(1) + q.send(@method, 1, timeout: 0).should == q + q.send(@method, 2, timeout: 0).should == nil + end + + it "returns nil if no space is available in time" do q = @object.call(1) q << 1 t = Thread.new { diff --git a/thread_sync.c b/thread_sync.c index 4ae404ec05..b2ee052aa5 100644 --- a/thread_sync.c +++ b/thread_sync.c @@ -1029,13 +1029,19 @@ static VALUE https://github.com/ruby/ruby/blob/trunk/thread_sync.c#L1029 queue_do_pop(VALUE self, struct rb_queue *q, int should_block, VALUE timeout) { check_array(self, q->que); - rb_hrtime_t end = queue_timeout2hrtime(timeout); + if (RARRAY_LEN(q->que) == 0) { + if (!should_block) { + rb_raise(rb_eThreadError, "queue empty"); + } + + if (RTEST(rb_equal(INT2FIX(0), timeout))) { + return Qnil; + } + } + rb_hrtime_t end = queue_timeout2hrtime(timeout); while (RARRAY_LEN(q->que) == 0) { - if (!should_block) { - rb_raise(rb_eThreadError, "queue empty"); - } - else if (queue_closed_p(self)) { + if (queue_closed_p(self)) { return queue_closed_result(self, q); } else { @@ -1232,16 +1238,22 @@ rb_szqueue_max_set(VALUE self, VALUE vmax) https://github.com/ruby/ruby/blob/trunk/thread_sync.c#L1238 static VALUE rb_szqueue_push(rb_execution_context_t *ec, VALUE self, VALUE object, VALUE non_block, VALUE timeout) { - rb_hrtime_t end = queue_timeout2hrtime(timeout); - bool timed_out = false; struct rb_szqueue *sq = szqueue_ptr(self); + if (queue_length(self, &sq->q) >= sq->max) { + if (RTEST(non_block)) { + rb_raise(rb_eThreadError, "queue full"); + } + + if (RTEST(rb_equal(INT2FIX(0), timeout))) { + return Qnil; + } + } + + rb_hrtime_t end = queue_timeout2hrtime(timeout); while (queue_length(self, &sq->q) >= sq->max) { - if (RTEST(non_block)) { - rb_raise(rb_eThreadError, "queue full"); - } - else if (queue_closed_p(self)) { - break; + if (queue_closed_p(self)) { + raise_closed_queue_error(self); } else { rb_execution_context_t *ec = GET_EC(); @@ -1262,18 +1274,11 @@ rb_szqueue_push(rb_execution_context_t *ec, VALUE self, VALUE object, VALUE non_ https://github.com/ruby/ruby/blob/trunk/thread_sync.c#L1274 }; rb_ensure(queue_sleep, (VALUE)&queue_sleep_arg, szqueue_sleep_done, (VALUE)&queue_waiter); if (!NIL_P(timeout) && rb_hrtime_now() >= end) { - timed_out = true; - break; + return Qnil; } } } - if (queue_closed_p(self)) { - raise_closed_queue_error(self); - } - - if (timed_out) return Qnil; - return queue_do_push(self, &sq->q, object); } diff --git a/thread_sync.rb b/thread_sync.rb index 7e4c341ad0..f8fa69900b 100644 --- a/thread_sync.rb +++ b/thread_sync.rb @@ -10,7 +10,7 @@ class Thread https://github.com/ruby/ruby/blob/trunk/thread_sync.rb#L10 # +ThreadError+ is raised. # # If +timeout+ seconds have passed and no data is available +nil+ is - # returned. + # returned. If +timeout+ is +0+ it returns immediately. def pop(non_block = false, timeout: nil) if non_block && timeout raise ArgumentError, "can't set a timeout if non_block is enabled" @@ -32,7 +32,7 @@ class Thread https://github.com/ruby/ruby/blob/trunk/thread_sync.rb#L32 # suspended, and +ThreadError+ is raised. # # If +timeout+ seconds have passed and no data is available +nil+ is - # returned. + # returned. If +timeout+ is +0+ it returns immediately. def pop(non_block = false, timeout: nil) if non_block && timeout raise ArgumentError, "can't set a timeout if non_block is enabled" @@ -54,7 +54,7 @@ class Thread https://github.com/ruby/ruby/blob/trunk/thread_sync.rb#L54 # thread isn't suspended, and +ThreadError+ is raised. # # If +timeout+ seconds have passed and no space is available +nil+ is - # returned. + # returned. If +timeout+ is +0+ it returns immediately. # Otherwise it returns +self+. def push(object, non_block = false, timeout: nil) if non_block && timeout -- cgit v1.2.3 -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/