ruby-core

Issue #19473 has been updated by Eregon (Benoit Daloze).


ko1 (Koichi Sasada) wrote in #note-11:
> My understandings are:
> 
> (1) It is hard to detect such deadlock risk because such errors occur with with very low frequency.
> (2) It is hard to expect that the Mutex users care about signal handlers.
> (3) It is hard to modify fixing with signal handlers.
> 
> So I think current limitation makes sense.

No, it doesn't make sense, because it actually prevents the proper fix for `TIMEOUT_THREAD_MUTEX` for Timeout:
https://github.com/ruby/timeout/issues/17#issuecomment-1464039853
So the limitation is unsound (it prevents valid and correct usages) and so we must remove it.

Also it doesn't catch `try_lock` and `Queue#pop`, etc, so it's very rough and inaccurate.

We could make it a `$VERBOSE = true` warning maybe, but again it would be false positives and impossible to address, so it seems not good.

Another idea would be being able to mark which Mutex are "trap-safe", maybe with an argument to Mutex#initialize.
Sort of like how IO does it.

ko1 (Koichi Sasada) wrote in #note-11:
> akr's idea is not about Timeout library, but for Timeout users in trap handers.
> If someone wants to use Timeout (or other complex/thread-safety needed code), making a thread like `trap(...){Thread.new{ ... }}` seems feasible workaround.

But users might use some gem or some library code in trap handler and they don't know whether that might not know whether that uses Timeout or not.
And even if they know, it should just work, without needing any workaround from the user.

My conclusion is doing nothing is unacceptable, this CRuby limitation breaks valid and correct code such as https://github.com/ruby/timeout/issues/17#issuecomment-1464039853.
So either:
1) We remove this unsound limitation, same as on other Rubies
2) We add an argument to Mutex#initialize to mark it as trap-safe
3) We execute trap handlers on a separate thread

Which one do we choose?

I think 3 is the best (much easier to reason about), but of course there is some potential for incompatibility there.
1 or 2 seem easy, so we could do them fast and maybe even backport it.

----------------------------------------
Bug #19473: can't be called from trap context (ThreadError) is too limiting
https://bugs.ruby-lang.org/issues/19473#change-102369

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* ruby -v: ruby 3.2.1 (2023-02-08 revision 31819e82c8) [x86_64-linux]
* Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN
----------------------------------------
Simple reproducer:
```
$ ruby -ve 'm=Mutex.new; trap(:HUP) { m.synchronize { p :OK } }; Process.kill :HUP, Process.pid; sleep 0.1'
ruby 3.2.1 (2023-02-08 revision 31819e82c8) [x86_64-linux]
-e:1:in `synchronize': can't be called from trap context (ThreadError)
	from -e:1:in `block in <main>'
	from -e:1:in `kill'
	from -e:1:in `<main>'
```

Expected behavior:
```
$ ruby -ve 'm=Mutex.new; trap(:HUP) { m.synchronize { p :OK } }; Process.kill :HUP, Process.pid; sleep 0.1'
truffleruby 22.3.1, like ruby 3.0.3, GraalVM CE Native [x86_64-linux]
:OK

$ ruby -ve 'm=Mutex.new; trap(:HUP) { m.synchronize { p :OK } }; Process.kill :HUP, Process.pid; sleep 0.1'
jruby 9.4.0.0 (3.1.0) 2022-11-23 95c0ec159f OpenJDK 64-Bit Server VM 17.0.6+10 on 17.0.6+10 +jit [x86_64-linux]
:OK
```

This exception is highly problematic, for instance it breaks `Timeout.timeout` in `trap`:
https://github.com/ruby/timeout/issues/17#issuecomment-1142035939

I suppose this behavior is because *sometimes* it's problematic to lock a Mutex in trap, e.g., if it's already locked by the main thread/fiber.
But that would otherwise already raise `deadlock; recursive locking (ThreadError)`, so there is no point to fail early.
And that's just one case, not all, so we should not always raise an exception.

There seems to be no valid reason to prevent *all* `Mutex#synchronize` in `trap`.
After all, if the Mutex for instance is only used in `trap`, it's well-defined AFAIK.
For instance a given trap handler does not seem executed concurrently:
```
$ ruby -ve 'trap(:HUP) { puts "in trap\n"+caller.join("\n")+"\n\n"; sleep 0.1 }; pid = Process.pid; Process.wait fork { 20.times { Process.kill :HUP, pid } }; sleep 1'
ruby 3.2.1 (2023-02-08 revision 31819e82c8) [x86_64-linux]
in trap
-e:1:in `wait'
-e:1:in `<main>'

in trap
-e:1:in `wait'
-e:1:in `<main>'

in trap
-e:1:in `wait'
-e:1:in `<main>'

in trap
-e:1:in `wait'
-e:1:in `<main>'

in trap
-e:1:in `wait'
-e:1:in `<main>'

in trap
-e:1:in `wait'
-e:1:in `<main>'

```

And if the trap handler using the Mutex is never called while the Mutex is held by the main thread/fiber, there is also no problem.



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- [email protected]
 To unsubscribe send an email to [email protected]
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/

Thread

Prev Next

In This Thread

Prev Next