Bug #16608
closedConditionVariable#wait should return false when timeout exceeded
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 nobu (Nobuyoshi Nakada) over 4 years ago
How about https://github.com/ruby/ruby/pull/2884
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) about 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) about 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) about 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.
Updated by nobu (Nobuyoshi Nakada) about 3 years ago
- Status changed from Assigned to Closed
Applied in changeset git|070557afc4ca83876b951fe090806b59e3867ae5.
Distinguish signal and timeout [Bug #16608]