`Concurrent::Promises::Future#value!` can return `nil` even when promise is not resolved yet
907th opened this issue · 6 comments
Details
* Operating system: mac
* Ruby implementation: Ruby
* `concurrent-ruby` version: 1.2.2
* `concurrent-ruby-ext` installed: no
* `concurrent-ruby-edge` used: no
When main process received SIGINT currently running future.value! returns nil but future.pending? is true and it can be resolved later.
The documentation tells us:
Returns:
(Object, nil, timeout_value) — the value of the Future when fulfilled, or nil on rejection, or timeout_value on timeout.
So I think either the documentation needs to be updated or the signal handling needs to be fixed.
Background
I worked on a code base which used chunks of code like:
task1, task2, ... = Concurrent::Promises.future { ... }, ...
value1, value2, ... = task1.value!, ...
work_with(value1, value2, ...)It was written in the assumption that nil value means rejection.
I wanted to implement SIGINT handling in the main thread and provide this code with the gracefull shutdown feature. But this code started to report future rejections (false positives) after sending SIGINT to the main process.
To reproduce:
# test.rb
require "concurrent-ruby"
Signal.trap("INT") {}
task = Concurrent::Promises.future do
i = 0
loop do
i = (i + 1) % 10
end
i
end
puts task.value!
puts "Unexpected behaviour?" if task.pending?Run in console:
❯ ruby ./test.rb
^C
Unexpected behaviour?
Thanks for the report.
That's very surprising, as Signal.trap("INT") {} should mean sending SIGINT should do nothing.
But actually with a little debugging what happens is ConditionVariable#wait has a spurious wakeup on CRuby (.wait returns even though neither .broadcast nor .signal has been called) in that case, probably because CRuby does SIGVTALRM/interrupt logic anyway it seems, even with the trap overridden.
Although even though the trap is overridden, it still has to execute that empty block on the main thread, so that's probably why we get that internal interrupt.
That's maybe a CRuby issue, because for instance there is no such issue on TruffleRuby.
We can workaround in
to use
until instead of unless.In most cases there should always be a loop around a ConditionVariable#wait, but since promises have a single state change that shouldn't be necessary.
CRuby issue for this: https://bugs.ruby-lang.org/issues/20047
@eregon what are you thoughts about making a private/internal class like ReliableConditionVariable that would wrap the loop/timeout fix for CRuby, or defer to the language recommendation? I wonder how many places this pattern should be duplicated... though hopefully a fix gets some traction in upstream CRuby too.
I reviewed all usages of ConditionVariable#wait in concurrent-ruby, they seem all fine.
In addition to #1016 there was also #1017 and #1018.
But as can be seen in #1017 sometimes it should not be a loop directly around the .wait.
So given we need the code only in one place it feels like the class is not necessary.
BTW, there is already