Project

General

Profile

Bug #15992

An exception breaks monitor state and cause deadlock

Added by naruse (Yui NARUSE) 3 months ago. Updated about 2 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
[ruby-core:93652]

Description

lib/monitor.rb provides Monitor.
But its state handling is weak for interrupts caused by Thread.kill for example timeout libraries.

Timeout exception may happen everywhere. If it raised when the thread is executing

  def mon_exit
    mon_check_owner
    @mon_count -=1
    if @mon_count == 0
      @mon_owner = nil
      # HERE!!!
      @mon_mutex.unlock
    end
  end

It breaks the state of the monitor and it causes deadlock.

Associated revisions

Revision f91879a7
Added by naruse (Yui NARUSE) 3 months ago

handle_interrupt to defend monitor state [Bug #15992]

If an exception is raised from another thread for example Timeout
and this thread is just after mon_exit's @mon_owner = nil,
the exception breaks the state of MonitorMixin. To prevent that situation,
it need to block interruption in mon_enter and mon_exit.

Revision 9ab43c7a
Added by nagachika (Tomoyuki Chikanaga) about 2 months ago

merge revision(s) f91879a7b548284c93743168acfd11e3d2aeefac: [Backport #15992]

    handle_interrupt to defend monitor state [Bug #15992]

    If an exception is raised from another thread for example Timeout
    and this thread is just after `mon_exit`'s `@mon_owner = nil`,
    the exception breaks the state of MonitorMixin. To prevent that situation,
    it need to block interruption in mon_enter and mon_exit.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_6@67742 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 67742
Added by nagachika (Tomoyuki Chikanaga) about 2 months ago

merge revision(s) f91879a7b548284c93743168acfd11e3d2aeefac: [Backport #15992]

handle_interrupt to defend monitor state [Bug #15992]

If an exception is raised from another thread for example Timeout
and this thread is just after `mon_exit`'s `@mon_owner = nil`,
the exception breaks the state of MonitorMixin. To prevent that situation,
it need to block interruption in mon_enter and mon_exit.

Revision fbb96f1b
Added by nagachika (Tomoyuki Chikanaga) about 2 months ago

merge revision(s) 9557069299ac3b96691040a541afa65761a724ad: [Backport #15992]

    Avoid creating Hash objects per each mon_synchronize call (#2393)

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_6@67749 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 67749
Added by nagachika (Tomoyuki Chikanaga) about 2 months ago

merge revision(s) 9557069299ac3b96691040a541afa65761a724ad: [Backport #15992]

Avoid creating Hash objects per each mon_synchronize call (#2393)

Revision 4d9c5875
Added by usa (Usaku NAKAMURA) about 2 months ago

merge revision(s) f91879a7b548284c93743168acfd11e3d2aeefac,9557069299ac3b96691040a541afa65761a724ad: [Backport #15992]

    handle_interrupt to defend monitor state [Bug #15992]

    If an exception is raised from another thread for example Timeout
    and this thread is just after `mon_exit`'s `@mon_owner = nil`,
    the exception breaks the state of MonitorMixin. To prevent that situation,
    it need to block interruption in mon_enter and mon_exit.

    Avoid creating Hash objects per each mon_synchronize call (#2393)

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_5@67774 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 67774
Added by usa (Usaku NAKAMURA) about 2 months ago

merge revision(s) f91879a7b548284c93743168acfd11e3d2aeefac,9557069299ac3b96691040a541afa65761a724ad: [Backport #15992]

handle_interrupt to defend monitor state [Bug #15992]

If an exception is raised from another thread for example Timeout
and this thread is just after `mon_exit`'s `@mon_owner = nil`,
the exception breaks the state of MonitorMixin. To prevent that situation,
it need to block interruption in mon_enter and mon_exit.

Avoid creating Hash objects per each mon_synchronize call (#2393)

History

#1

Updated by naruse (Yui NARUSE) 3 months ago

  • Status changed from Open to Closed

Applied in changeset git|f91879a7b548284c93743168acfd11e3d2aeefac.


handle_interrupt to defend monitor state [Bug #15992]

If an exception is raised from another thread for example Timeout
and this thread is just after mon_exit's @mon_owner = nil,
the exception breaks the state of MonitorMixin. To prevent that situation,
it need to block interruption in mon_enter and mon_exit.

Updated by nagachika (Tomoyuki Chikanaga) about 2 months ago

  • Backport changed from 2.5: REQUIRED, 2.6: REQUIRED to 2.5: REQUIRED, 2.6: DONE

ruby_2_6 r67742 merged revision(s) f91879a7b548284c93743168acfd11e3d2aeefac.

Updated by nagachika (Tomoyuki Chikanaga) about 2 months ago

  • Backport changed from 2.5: REQUIRED, 2.6: DONE to 2.5: REQUIRED, 2.6: REQUIRED

9557069299ac3b96691040a541afa65761a724ad is a follow up commit to ease memory consumption impact of f91879a7b548284c93743168acfd11e3d2aeefac. I think it should be backported too.

Updated by nagachika (Tomoyuki Chikanaga) about 2 months ago

  • Backport changed from 2.5: REQUIRED, 2.6: REQUIRED to 2.5: REQUIRED, 2.6: DONE

ruby_2_6 r67749 merged revision(s) 9557069299ac3b96691040a541afa65761a724ad.

Updated by usa (Usaku NAKAMURA) about 2 months ago

  • Backport changed from 2.5: REQUIRED, 2.6: DONE to 2.5: DONE, 2.6: DONE

ruby_2_5 r67774 merged revision(s) f91879a7b548284c93743168acfd11e3d2aeefac,9557069299ac3b96691040a541afa65761a724ad.

Updated by Eregon (Benoit Daloze) about 2 months ago

Should mon_enter and mon_exit use Thread.handle_interrupt inside their method definitions, so they are safe when called directly too?

Thread.handle_interrupt(Exception => :never) seems too much for mon_enter, I think Thread.handle_interrupt(Exception => :on_blocking) would be better.
Otherwise, it's not possible to interrupt a thread trying to get a contended monitor with Thread#raise:

ruby -rmonitor -e 'o=Object.new.extend(MonitorMixin); Thread.new{ o.mon_enter; sleep }; sleep 0.1; Thread.new{sleep 3; p :kill; Thread.main.raise "bye"}; p o.synchronize { p :in; sleep }'

Never exits on trunk, but it does exit on 2.6.2 and before. Even Thread#kill does not help.

Ctrl+C does interrupt it, because signal traps are not yet supported by handle_interrupt, but that might change.

Also available in: Atom PDF