Bug #9343

[PATCH] SizedQueue#max= wakes up waiters properly

Added by Eric Wong 4 months ago. Updated 2 months ago.

[ruby-core:59466]
Status:Closed
Priority:Normal
Assignee:Koichi Sasada
Category:ext
Target version:current: 2.2.0
ruby -v:- Backport:1.9.3: DONTNEED, 2.0.0: DONTNEED, 2.1: DONE

Description

We were accessing the wrong array and trying to wake up
elements stored in the queue :x

$ git pull git://80x24.org/ruby.git szqmaxset-wakeup

The following changes since commit d1cc0ebb38a23feb37bb16ff2df3137c3cead069:

mkrunnable.rb: fix DLL path on Windows (2014-01-02 02:41:06 +0000)

are available in the git repository at:

git://80x24.org/ruby.git szqmaxset-wakeup

for you to fetch changes up to 9932853dec14a4f62606098d452da80a54bf9ecc:

SizedQueue#max= wakes up waiters properly (2014-01-02 07:22:02 +0000)


Eric Wong (1):
SizedQueue#max= wakes up waiters properly

ext/thread/thread.c | 2 +-
test/thread/test_queue.rb | 7 +++++++
2 files changed, 8 insertions(+), 1 deletion(-)

0001-SizedQueue-max-wakes-up-waiters-properly.patch Magnifier (1.44 KB) Eric Wong, 01/02/2014 04:25 PM

Associated revisions

Revision 44852
Added by normal 3 months ago

ext/thread: SizedQueue#max= wakes up waiters properly

* ext/thread/thread.c (rb_szqueue_max_set): use correct queue and
  limit wakeups.  [Bug #9343]
* test/thread/test_queue.rb (test_sized_queue_assign_max):
  test for bug

History

#1 Updated by Koichi Sasada 4 months ago

  • Assignee set to Koichi Sasada

#2 Updated by Koichi Sasada 3 months ago

sorry for long absent.

I reviewed this patch and I have a question.
Only `diff' threads should wake-up?

Index: thread.c
===================================================================
--- thread.c    (revision 44833)
+++ thread.c    (working copy)
@@ -437,7 +437,7 @@
    diff = max - GET_SZQUEUE_ULONGMAX(self);
     }
     RSTRUCT_SET(self, SZQUEUE_MAX, vmax);
-    while (diff > 0 && !NIL_P(t = rb_ary_shift(GET_QUEUE_QUE(self)))) {
+    while (diff-- > 0 && !NIL_P(t = rb_ary_shift(GET_SZQUEUE_WAITERS(self)))) {
    rb_thread_wakeup_alive(t);
     }
     return vmax;

#3 Updated by Eric Wong 3 months ago

ko1@atdot.net wrote:

sorry for long absent.

No worries, I forgot about this, too :x

I reviewed this patch and I have a question.
Only `diff' threads should wake-up?

Yes, your patch looks correct. Old thread.rb had "diff.times do"

#4 Updated by Koichi Sasada 3 months ago

  • ruby -v changed from ruby 2.2.0dev (2014-01-02 trunk 44484) [x86_64-linux] to -

(2014/02/06 2:53), normalperson@yhbt.net wrote:

Yes, your patch looks correct. Old thread.rb had "diff.times do"

All right.

Could you commit it?

--
// SASADA Koichi at atdot dot net

#5 Updated by Anonymous 3 months ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

Applied in changeset r44852.


ext/thread: SizedQueue#max= wakes up waiters properly

* ext/thread/thread.c (rb_szqueue_max_set): use correct queue and
  limit wakeups.  [Bug #9343]
* test/thread/test_queue.rb (test_sized_queue_assign_max):
  test for bug

#6 Updated by Eric Wong 3 months ago

SASADA Koichi ko1@atdot.net wrote:

Could you commit it?

Done, r44852. This needs a backport to 2.1

#7 Updated by Eric Hodel 3 months ago

  • Backport changed from 1.9.3: UNKNOWN, 2.0.0: UNKNOWN, 2.1: UNKNOWN to 1.9.3: DONTNEED, 2.0.0: DONTNEED, 2.1: REQUIRED

Update backport status

#8 Updated by Yui NARUSE 2 months ago

  • Backport changed from 1.9.3: DONTNEED, 2.0.0: DONTNEED, 2.1: REQUIRED to 1.9.3: DONTNEED, 2.0.0: DONTNEED, 2.1: DONE

Also available in: Atom PDF