[Ironruby-core] Code Review: Thread#raise

Curt Hagenlocher curth at microsoft.com
Tue Dec 30 16:29:08 EST 2008


Ah, I see; I misunderstood the way that flag was working.

-----Original Message-----
From: Shri Borde 
Sent: Tuesday, December 30, 2008 1:23 PM
To: Curt Hagenlocher; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Thread#raise

The terminology I am using throughout is synchronous exception would be a normal Kernel.raise as the thread knows exactly when and where the exception will occur. Thread#raise is considered to raise an exception asynchronously as you cannot control exactly when and where the exception will actually be raised on the target thread. So with this terminology, the stress mode should stay as "...ForSyncRaise" so that Thread.Abort is used even for Kernel.raise.

I will change Thread.raise with no arguments to inject a RuntimeError. I am not sure if its any better or worse at it depends on whether its preferable to fail early or to try to keep going. Failing early is usually a good idea, but in this case, given all the caveats about Thread#raise, I don't feel strongly.

Thanks,
Shri


-----Original Message-----
From: Curt Hagenlocher 
Sent: Tuesday, December 30, 2008 1: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




More information about the Ironruby-core mailing list