[ruby-core:72382] [Ruby trunk - Bug #11822] Semantics of Queue#pop after close are wrong

[headius@headius.com]

Issue #11822 has been updated by Charles Nutter.


tldr: Atomicity can be achieved with full locking, but it prevents Queue from being lock-free in the future. #pop should have a way to indicate the queue is closed other than just returning nil. I don't see anything that prevents the current impl of #close from going into 2.3.

Atomicity
-----------

It is possible to implement these operations with a hard lock, but my concern is that it prevents us from using more efficient lock-free Queue implementations in the future.

I have reimplemented JRuby's Queue to mimic a GVL by locking around all operations, and it is passing tests. Performance seems fine, but there were other changes that muddle measurement a bit.

Queue#pop behavior on a closed queue
----------------------------------------------

I do have concerns about #pop returning nil on a closed queue, but it is not an implementation challenge...just an API challenge. The concern is this: there's no indication from #pop that the queue is closed, since nil is a valid value to push into a queue. Or put another way: it will no longer be safe to have nil be a regular value in a queue, since it could now mean the queue is closed. You will have to check #closed? every time.

Perhaps there should be a way to query if the queue is closed *and* pop at the same time? Go provides a multiple assignment form of receive:

x, ok = <-ch

where "ok" receives a boolean indicating that the queue is closed.

We don't want to make pop always return multiple values, so here's a couple ideas for how we might do this in Ruby:

(Queue#pop already takes a boolean value to indicate if it should be nonblocking, so overloading Queue#pop is not an option)

```ruby
q.pop!([nonblock]) # raises exception on a closed queue
q.pop?(value [, nonblock]) # returns value if queue is closed (or would block?), rather than nil
```

----------------------------------------
Bug #11822: Semantics of Queue#pop after close are wrong
https://bugs.ruby-lang.org/issues/11822#change-55671

* Author: Charles Nutter
* Status: Open
* Priority: Normal
* Assignee: Koichi Sasada
* ruby -v: 
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
Current test/ruby/thread/test_queue.rb test_close has the following assertion that seems wrong to me:

```ruby
  def test_close
    [->{Queue.new}, ->{SizedQueue.new 3}].each do |qcreate|
      q = qcreate.call
      assert_equal false, q.closed?
      q << :something
      assert_equal q, q.close
      assert q.closed?
      assert_raise_with_message(ClosedQueueError, /closed/){q << :nothing}
      assert_equal q.pop, :something  # <<< THIS ONE
      assert_nil q.pop
      assert_nil q.pop
      # non-blocking
      assert_raise_with_message(ThreadError, /queue empty/){q.pop(non_block=true)}
    end
  end
```

Once a queue is closed, I don't think it should ever return a result anymore. The queue should be cleared and pop should always return nil.

In r52691, ko1 states that "deq'ing on closed queue returns nil, always." This test does not match that behavior.



-- 
https://bugs.ruby-lang.org/