[Ironruby-core] Code Review: Thread#raise

[Charles Oliver Nutter]

I think I'm coming around to your side. My position is current the 
following:

1. If critical officially remains specified as its current behavior in 
MRI, we should be warning people not to use it and should have a warning 
emitted at least once.
2. If we can get ruby-core to agree that critical should be expected to 
be no more than a global mutex and not necessarily deschedule threads, 
I'd be willing to make the same change in JRuby and omit the warning.

I definitely think we need to talk to ruby-core and open it up to the 
community, however, to see if there are cases where people depend on 
critical= actually descheduling. Shri: can you propose making the 
"global mutex" definition of critical the official one?

- Charlie

Shri Borde wrote:
> I agree that critical= can cause problems, and should go away. However, it can also be used in a reasonably safe way. Something like this. Such usages do not rely on other threads being frozen. Most uses seem to use such a pattern.
> 
> def get_unique_cookie()
>   begin
>     Thread.critical = true
>     cookie = @@counter
>     @@counter += 1
>   ensure
>     Thread.critical = false
>   end
>   cookie
> end
> 
> I am suggesting not adding a warning purely from a pragmatic perspective. The patter above is used in a lots of of places, and a warning could irritate users if they see it too often when using their favorite gems.
> 
> Is there a way for Ruby devs to control the warning level? If so, it will be a question of what level to put the warning under, not whether to add it or not.
> 
> Thanks,
> Shri
> 
> 
> -----Original Message-----
> From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Charles Oliver Nutter
> Sent: Tuesday, January 06, 2009 10:53 AM
> To: ironruby-core at rubyforge.org
> Cc: IronRuby External Code Reviewers; Tomas Matousek
> Subject: Re: [Ironruby-core] Code Review: Thread#raise
> 
> I think the implications of critical= in MRI are more complicated than 
> just a local lock. It actually stops normal thread scheduling, so other 
> threads freeze what they're doing. That's vastly more intrusive than 
> just a lock or critical section:
> 
> ◆ ruby -e "Thread.new { sleep 1; puts Time.now; redo }; sleep 3; 
> Thread.critical = true; puts 'critical'; sleep 3; puts 'leaving 
> critical'; Thread.critical = false; sleep 3"
> Tue Jan 06 18:50:34 +0000 2009
> Tue Jan 06 18:50:35 +0000 2009
> critical
> leaving critical
> Tue Jan 06 18:50:39 +0000 2009
> Tue Jan 06 18:50:40 +0000 2009
> Tue Jan 06 18:50:41 +0000 2009
> 
> If those other threads are holding locks or resources, deadlock is 
> extremely easy, and of course it's nearly impossible to emulate right 
> with real parallel threads since you can't easily stop them whenever you 
> want.
> 
> #kill and #raise are also very tricky and dangerous, but they're at 
> least localized. They can be defined in terms of a message passed from 
> one thread to another plus a periodic message checkpoint.
> 
> I still believe critical= is much more in need of a warning.
> 
> Shri Borde wrote:
>> Warning sounds reasonable for Thread#kill and Thread#raise. FWIW, Mongrel and webbrick do use Thread#kill to kill a background thread.
>>
>> Thread.critical= can actually be used in a sane way for simple synchronization, like the lock keyword in C#. This is used much more widely, and a warning for this will cause lot of false alarms.
>>
>> Thanks,
>> Shri
>>
>>
>> -----Original Message-----
>> From: Tomas Matousek
>> Sent: Saturday, January 03, 2009 11:52 AM
>> To: Curt Hagenlocher; Shri Borde; IronRuby External Code Reviewers
>> Cc: ironruby-core at rubyforge.org
>> Subject: RE: Code Review: Thread#raise
>>
>> I think we should at least report a warning when Thread#kill, Thread#raise or Thread#critical= is called if not eliminating them as Charlie proposed on his blog.
>>
>> Tomas
>>
>> -----Original Message-----
>> From: Curt Hagenlocher
>> Sent: Tuesday, December 30, 2008 10:05 PM
>> To: Shri Borde; IronRuby External Code Reviewers
>> Cc: ironruby-core at rubyforge.org
>> Subject: RE: Code Review: Thread#raise
>>
>> Shouldn't the option "UseThreadAbortForSyncRaise" be called "...ForASyncRaise"?
>> I think that Thread.raise with no arguments should just inject a RuntimeError with no message as if $! were nil; this makes more sense than failing.  Trying to reference a "current exception" in another thread is a scary operation even if that's what MRI is doing.
>>
>> Other than that, changes look really nice.  But anyone thinking of using this functionality should read Charlie's excellent piece from earlier in the year: http://blog.headius.com/2008/02/rubys-threadraise-threadkill-timeoutrb.html
>>
>> -----Original Message-----
>> From: Shri Borde
>> Sent: Monday, December 29, 2008 3:00 PM
>> To: IronRuby External Code Reviewers
>> Cc: ironruby-core at rubyforge.org
>> Subject: Code Review: Thread#raise
>>
>>   tfpt review "/shelveset:raise;REDMOND\sborde"
>>   Comment  :
>>   Implements Thread#raise using Thread.Abort, and added tests for it
>>   Implemented stress mode (RubyOptions.UseThreadAbortForSyncRaise) which found more issues. Fixed most but not all
>>   Enabled test for timeout as well
>>   Remaining work (not high pri for now)
>>    - Thread#raise without Exception parameters is not supported as it needs to access the active exception of the target thread. This is stored as a thread-local static, and so cannot be accessed from other threads. Can be fixed by not using ThreadStaticAttribute.
>>    - Adding probes (in generated code, in C# library code, etc) will help to raise the exception quicker as Thread.Abort can be delayed indefinitely. Ideally, we need both approaches. For now, using Thread.Abort goes a long way.
>>    - Ideally, we would add a try-catch to the IDynamicObject/MetaObject code paths so that when other languages called into Ruby code, they would get the expected user exception rather than a ThreadAbortException
>>
>>   RunRSpec: supports -ruby to run with MRI. Its much faster than doing "rake ruby why_regression". Added support for -e to run a specific example
>>
>>
>> _______________________________________________
>> Ironruby-core mailing list
>> Ironruby-core at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/ironruby-core
> 
> _______________________________________________
> Ironruby-core mailing list
> Ironruby-core at rubyforge.org
> http://rubyforge.org/mailman/listinfo/ironruby-core
> _______________________________________________
> Ironruby-core mailing list
> Ironruby-core at rubyforge.org
> http://rubyforge.org/mailman/listinfo/ironruby-core