From 60defe0a68a40d1b3225cf6b971ea195e19ae2e2 Mon Sep 17 00:00:00 2001
From: Jean Boussier <jean.boussier@gmail.com>
Date: Thu, 6 Oct 2022 15:53:16 +0200
Subject: [PATCH] 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
         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
         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
 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)
 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_
             };
             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
     # +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
     # 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
     # 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