Project

General

Profile

Actions

Bug #16608

closed

ConditionVariable#wait should return false when timeout exceeded

Added by shugo (Shugo Maeda) over 4 years ago. Updated over 3 years ago.

Status:
Closed
Target version:
-
[ruby-core:97063]

Description

The following program prints false on Ruby 1.8, but true on Ruby 1.9 or later.

require "monitor"
m = Monitor.new
c = m.new_cond
m.synchronize { p c.wait(0.1) }

However, it's not critical because most programs check the condition after wait.

Updated by shugo (Shugo Maeda) over 4 years ago

  • Status changed from Open to Assigned
  • Assignee set to shugo (Shugo Maeda)

Updated by shugo (Shugo Maeda) over 4 years ago

  • Assignee changed from shugo (Shugo Maeda) to nobu (Nobuyoshi Nakada)

nobu (Nobuyoshi Nakada) wrote in #note-2:

How about https://github.com/ruby/ruby/pull/2884

ko1 suggested Mutex#release (new version of Mutex#sleep) for backward compatibility.
What do you think of it?

Updated by Eregon (Benoit Daloze) over 4 years ago

shugo (Shugo Maeda) wrote in #note-3:

ko1 suggested Mutex#release (new version of Mutex#sleep) for backward compatibility.

release sounds like unlock as in acquire/release is similar to lock/unlock.
So I think we need a different name there.

I'm not sure about backward compatibility, but I would think it's fine to change the return value here as probably almost nothing uses it.

Updated by jeremyevans0 (Jeremy Evans) over 4 years ago

I agree with @Eregon (Benoit Daloze) that we can probably change the return value of ConditionVariable#wait in Ruby 3. As it always returns true currently, it's worthless and nothing should be relying on it.

I can see that changing Mutex#sleep to start returning nil could break things, though. I think we do need a new method. Maybe Mutex#sleep_for or Mutex#sleep!.

Updated by jeremyevans0 (Jeremy Evans) over 3 years ago

I've added a pull request that builds on @nobu's pull request, but uses a new method (Mutex#sleep_for) instead of making backwards-incompatible changes to Mutex#sleep: https://github.com/ruby/ruby/pull/4256

Updated by jeremyevans0 (Jeremy Evans) over 3 years ago

@matz (Yukihiro Matsumoto) agreed to change the return value of Mutex#sleep at the last developer meeting. However, that breaks a Mutex_m test: https://github.com/ruby/ruby/pull/4256/checks?check_run_id=2135513377#step:15:1295

So the Mutex_m test needs to be fixed first, a new release needs to be made, and the bundled gems need to be updated, before the Mutex#sleep change can be merged.

Updated by Eregon (Benoit Daloze) over 3 years ago

That looks like an error of rbs actually, cc @soutaro (Soutaro Matsumoto)

Updated by ko1 (Koichi Sasada) over 3 years ago

Previous dev-meeting (March), there is no objection to change the return value of Mutex#sleep.

matz: agreed to change the return value of Mutex#sleep.
naruse: agreed to change in Ruby 3.1 because this is so detail

Congrats!

Updated by jeremyevans0 (Jeremy Evans) over 3 years ago

  • Assignee changed from nobu (Nobuyoshi Nakada) to soutaro (Soutaro Matsumoto)

I submitted a pull request to rbs to fix the failure (https://github.com/ruby/rbs/pull/683) I also updated the ruby pull request (https://github.com/ruby/ruby/pull/4256) to rebase it on the current master branch. Once the rbs pull request is merged and a release is made, we should be able to merge the ruby pull request.

Updated by jeremyevans0 (Jeremy Evans) over 3 years ago

The rbs pull request has been merged. So we just need to wait for the next rbs gem release and for bundled gems to be updated, then we can merge the pull request.

Actions #12

Updated by nobu (Nobuyoshi Nakada) over 3 years ago

  • Status changed from Assigned to Closed

Applied in changeset git|070557afc4ca83876b951fe090806b59e3867ae5.


Distinguish signal and timeout [Bug #16608]

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0