Feature #15350: [PATCH] thread_sync.c (queue_sleep): remove deadlock checking

• Status: Open
• Assignee: -
• Target version: -

[normalperson (Eric Wong)]

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)

[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?

[normalperson (Eric Wong)]

[e-mail] 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.

[jeremyevans0 (Jeremy Evans)]

normalperson (Eric Wong) wrote:

[e-mail] 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:

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

[normalperson (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.

[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 is to forget to yield the result of a yield_call. This blocks the call forever:

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.

[normalperson (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.