Project

General

Profile

Actions

Bug #18649

closed

Enumerable#first breaks out of the incorect block when across threads

Added by Eregon (Benoit Daloze) 5 months ago. Updated 4 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [x86_64-linux]
[ruby-core:107980]

Description

def synchronize
  yield
end

def execute(task)
  success = true
  value = reason = nil
  end_sync = false

  synchronize do
    begin
      p :before
      value = task.call
      p :never_reached
      success = true
    rescue StandardError => ex
      p [:rescue, ex]
      reason = ex
      success = false
    end

    end_sync = true
    p :end_sync
  end

  p :should_not_reach_here! unless end_sync
  [success, value, reason]
end

def foo
  Thread.new do
    result = execute(-> { yield 42 })
    p [:result, result]
  end.join
end

p [:first, to_enum(:foo).first]

This code should raise LocalJumpError (and that should get rescue'd) because Enumerable#first can't break/return across threads.
But instead, it seems to break out of the block given to synchronize, which is clearly wrong.
That case is shown as :should_not_reach_here!.

Results:

ruby 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [x86_64-linux]
:before
:should_not_reach_here!
[:result, [true, nil, nil]]
[:first, 42]

ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [x86_64-linux]
:before
:should_not_reach_here!
[:result, [true, nil, nil]]
[:first, 42]

CRuby (3.0 and 3.1) print :should_not_reach_here!, which is a semantic bug, if we get to the end of execute we should have gotten to the end of the block given to synchronize.

This is related to #18474 and https://github.com/ruby-concurrency/concurrent-ruby/issues/931.


Related issues 1 (0 open1 closed)

Related to Ruby master - Bug #18474: 938e027c seems to have caused a regression in yield handling with concurrent-rubyClosedActions
Actions #1

Updated by Eregon (Benoit Daloze) 5 months ago

  • Related to Bug #18474: 938e027c seems to have caused a regression in yield handling with concurrent-ruby added

Updated by jeremyevans0 (Jeremy Evans) 5 months ago

Looks like this bug started in Ruby 2.2:

$ ruby21 -v t/t55.rb
ruby 2.1.9p490 (2016-03-30 revision 54437) [x86_64-openbsd]
:before
t/t55.rb:34:in `join': unexpected break (LocalJumpError)
        from t/t55.rb:34:in `foo'
        from t/t55.rb:37:in `each'
        from t/t55.rb:37:in `first'
        from t/t55.rb:37:in `<main>'
$ ruby22 -v t/t55.rb
ruby 2.2.10p489 (2018-03-28 revision 63023) [x86_64-openbsd]
:before
:should_not_reach_here!
[:result, [true, nil, nil]]
[:first, 42]

I found out that the :should_not_reach_here! issue can be avoided by uncommenting a block of code added in 3d980e1643305ff2ef7492d5fe25d89f63b29268. This results in different behavior than pre-Ruby 2.2, and I'm not sure which is the desired behavior. With the block uncommented, the yielding thread raises LocalJumpError, but the calling thread gets the yielded value without an exception. I submitted a pull request for this approach: https://github.com/ruby/ruby/pull/5692

Updated by nobu (Nobuyoshi Nakada) 4 months ago

jeremyevans0 (Jeremy Evans) wrote in #note-2:

With the block uncommented, the yielding thread raises LocalJumpError, but the calling thread gets the yielded value without an exception.

Isn't it natural because the test code rescues but doesn't re-raise?

Updated by jeremyevans0 (Jeremy Evans) 4 months ago

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

jeremyevans0 (Jeremy Evans) wrote in #note-2:

With the block uncommented, the yielding thread raises LocalJumpError, but the calling thread gets the yielded value without an exception.

Isn't it natural because the test code rescues but doesn't re-raise?

I think the behavior of raising LocalJumpError is natural for the yielding thread (the yielding thread is the one that rescues but doesn't re-raise). What I'm not sure about is whether the calling thread should still be able to get the value for a cross-thread yield.

Updated by nobu (Nobuyoshi Nakada) 4 months ago

There is no way to tell whether the underlying each method is broken or exited gently.
I think that your PR would be better than the current.

Actions #6

Updated by jeremyevans (Jeremy Evans) 4 months ago

  • Status changed from Open to Closed

Applied in changeset git|4b14b2902abaa0e8f0d1e8282d2322f47431fa3f.


Uncomment code to raise LocalJumpError for yield across thread through enum

Not sure if this is the correct fix. It does raise LocalJumpError in
the yielding thread as you would expect, but the value yielded to the calling
thread is still yielded without an exception.

Fixes [Bug #18649]

Actions

Also available in: Atom PDF