[ruby-core:90099] [Ruby trunk Feature#15350] [PATCH] thread_sync.c (queue_sleep): remove deadlock checking

[ruby-core:90099] [Ruby trunk Feature#15350] [PATCH] thread_sync.c (queue_sleep): remove deadlock checking

[normalperson]

Issue #15350 has been reported by normalperson (Eric Wong).

----------------------------------------
Feature #15350: [PATCH] thread_sync.c (queue_sleep): remove deadlock checking
https://bugs.ruby-lang.org/issues/15350

* Author: normalperson (Eric Wong)
* Status: Open
* Priority: Normal
* Assignee: 
* Target version: 
----------------------------------------
thread_sync.c (queue_sleep): remove deadlock checking

Queue may be used inside signal handlers nowadays, so deadlock
checking is unnecessary and prevents single-threaded use.

We don't have deadlock checking for reading pipes/socket or
File#flock, either.


This IS a behavior change; but I don't think there is a risk
of incompatibility (aside from future code not working on old Rubies)


---Files--------------------------------
0001-thread_sync.c-queue_sleep-remove-deadlock-checking.patch (2.44 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:90101] [Ruby trunk Feature#15350] [PATCH] thread_sync.c (queue_sleep): remove deadlock checking

[merch-redmine]

Issue #15350 has been updated by jeremyevans0 (Jeremy Evans).


I expect this would make it more difficult to debug multithreaded use of Queue, since ruby would hang instead of raising a fatal exception as it does currently (assuming I understand the patch correctly).  As multithreaded use of Queue is probably more common than signal handler use of Queue, I'm not sure changing the default behavior is a good idea.

Could we potentially support this behavior via a keyword argument to `Queue#initialize` or as a separate Queue instance method, so that users who want to use Queue inside signal handlers could enable it for that specific Queue instance?

----------------------------------------
Feature #15350: [PATCH] thread_sync.c (queue_sleep): remove deadlock checking
https://bugs.ruby-lang.org/issues/15350#change-75223

* Author: normalperson (Eric Wong)
* Status: Open
* Priority: Normal
* Assignee: 
* Target version: 
----------------------------------------
thread_sync.c (queue_sleep): remove deadlock checking

Queue may be used inside signal handlers nowadays, so deadlock
checking is unnecessary and prevents single-threaded use.

We don't have deadlock checking for reading pipes/socket or
File#flock, either.


This IS a behavior change; but I don't think there is a risk
of incompatibility (aside from future code not working on old Rubies)


---Files--------------------------------
0001-thread_sync.c-queue_sleep-remove-deadlock-checking.patch (2.44 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:90107] Re: [Ruby trunk Feature#15350] [PATCH] thread_sync.c (queue_sleep): remove deadlock checking

[Eric Wong]

merch-redmine@jeremyevans.net wrote:
> I expect this would make it more difficult to debug
> multithreaded use of Queue, since ruby would hang instead of
> raising a fatal exception as it does currently (assuming I
> understand the patch correctly).  As multithreaded use of
> Queue is probably more common than signal handler use of
> Queue, I'm not sure changing the default behavior is a good
> idea.

I've never seen Queue usage as difficult to get right.  I see
some (not much) usefulness of deadlock checking for mutex and
CV; but Queue is like a pipe and there's already plenty of
opportunities for pipe-using code to deadlock.

> Could we potentially support this behavior via a keyword
> argument to `Queue#initialize` or as a separate Queue instance
> method, so that users who want to use Queue inside signal
> handlers could enable it for that specific Queue instance?

I considered transparently supporting this during Signal.trap
registration; but then realized it's not possible...
Well, what we could do is if ANY Signal.trap handler proc is
registered, we disable deadlock checking.

But I'm not sure if increasing API footprint for Queue is a good
idea, either.

[ruby-core:90108] [Ruby trunk Feature#15350] [PATCH] thread_sync.c (queue_sleep): remove deadlock checking

[merch-redmine]

Issue #15350 has been updated by jeremyevans0 (Jeremy Evans).


normalperson (Eric Wong) wrote:
> merch-redmine@jeremyevans.net wrote:
>  > I expect this would make it more difficult to debug
>  > multithreaded use of Queue, since ruby would hang instead of
>  > raising a fatal exception as it does currently (assuming I
>  > understand the patch correctly).  As multithreaded use of
>  > Queue is probably more common than signal handler use of
>  > Queue, I'm not sure changing the default behavior is a good
>  > idea.
>  
>  I've never seen Queue usage as difficult to get right.  I see
>  some (not much) usefulness of deadlock checking for mutex and
>  CV; but Queue is like a pipe and there's already plenty of
>  opportunities for pipe-using code to deadlock.

I use Queue pretty extensively when testing to ensure deterministic behavior in multithreaded code, and I can say it would be a lot more difficult to write such tests if a mistake caused the tests to deadlock instead of raising a fatal exception.

>  But I'm not sure if increasing API footprint for Queue is a good
>  idea, either.

That's a fair point.  However, considering you can work around the current deadlock detection with `Thread.new{loop{sleep(1000000)}}` before calling `Queue#pop`, the deadlock detection doesn't really cause major problems if you want to use Queue in signal handlers.  Example:

~~~ ruby
q = Queue.new
Signal.trap(:INT){q.push nil}
Thread.new{loop{sleep(1000000)}}
q.pop
~~~

----------------------------------------
Feature #15350: [PATCH] thread_sync.c (queue_sleep): remove deadlock checking
https://bugs.ruby-lang.org/issues/15350#change-75229

* Author: normalperson (Eric Wong)
* Status: Open
* Priority: Normal
* Assignee: 
* Target version: 
----------------------------------------
thread_sync.c (queue_sleep): remove deadlock checking

Queue may be used inside signal handlers nowadays, so deadlock
checking is unnecessary and prevents single-threaded use.

We don't have deadlock checking for reading pipes/socket or
File#flock, either.


This IS a behavior change; but I don't think there is a risk
of incompatibility (aside from future code not working on old Rubies)


---Files--------------------------------
0001-thread_sync.c-queue_sleep-remove-deadlock-checking.patch (2.44 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:90109] Re: [Ruby trunk Feature#15350] [PATCH] thread_sync.c (queue_sleep): remove deadlock checking

[Eric Wong]

> That's a fair point.  However, considering you can work around
> the current deadlock detection with
> `Thread.new{loop{sleep(1000000)}}` before calling `Queue#pop`,
> the deadlock detection doesn't really cause major problems if
> you want to use Queue in signal handlers.  Example:

Actually, your example is why I consider the existing deadlock
detection pretty pointless.

Also, one of my goals is to reduce the overhead of native
threads, so using an extra Thread defeats that goal.

[ruby-core:90290] [Ruby trunk Feature#15350] [PATCH] thread_sync.c (queue_sleep): remove deadlock checking

[lars]

Issue #15350 has been updated by larskanis (Lars Kanis).


I agree with Jeremy, that dropping the deadlock detection from `Queue#pop` would be pity. It's a lot easier if you get a clear error message, than debugging a starvation in your process.

A common mistake in [Eventbox](https://github.com/larskanis/eventbox) is to forget to yield the result of a `yield_call`. This blocks the call forever:

```ruby
class MyBox < Eventbox
  yield_call def init(result)
  end
end
MyBox.new
```

Fortunately is is immediately raised through a `fatal (No live threads left. Deadlock?)` error, since Eventbox uses a Queue internally in this case. And the stacktrace points to the blocked `yield_call`, so that the developer gets a good indication about what's missing.


----------------------------------------
Feature #15350: [PATCH] thread_sync.c (queue_sleep): remove deadlock checking
https://bugs.ruby-lang.org/issues/15350#change-75397

* Author: normalperson (Eric Wong)
* Status: Open
* Priority: Normal
* Assignee: 
* Target version: 
----------------------------------------
thread_sync.c (queue_sleep): remove deadlock checking

Queue may be used inside signal handlers nowadays, so deadlock
checking is unnecessary and prevents single-threaded use.

We don't have deadlock checking for reading pipes/socket or
File#flock, either.


This IS a behavior change; but I don't think there is a risk
of incompatibility (aside from future code not working on old Rubies)


---Files--------------------------------
0001-thread_sync.c-queue_sleep-remove-deadlock-checking.patch (2.44 KB)


-- 
https://bugs.ruby-lang.org/

[ruby-core:90306] Re: [Ruby trunk Feature#15350] [PATCH] thread_sync.c (queue_sleep): remove deadlock checking

[Eric Wong]

Noted.  I'll keep deadlock checking and see about implementing
it for auto-fiber (if I continue working on that).

I don't like deadlock checking at all (since I don't want a
nanny programming language), but I'll go along with it if that's
what users want.

But keep in mind that pipes/sockets are susceptible to the same
deadlocks; but people seem fine without deadlock checking there.