Ironruby core - Jan 2009 - Code Review: Issues with Thread#kill and raising exception from ensure clause

tfpt review "/shelveset:kill;REDMOND\sborde"

  Comment  : 
  Added more tests for Thread#kill, and fixed bugs that were exposed. Throwing
an exception from the ensure clause exposes new code paths.
  
  Ruby allows Thread#kill to be completely subverted (not just deferred as an
infinite loop in an ensure clause would do) by raising an exception from a
ensure clause. I have not matched that behavior, and left it as a IronRuby bug
for now.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: kill.diff
Type: application/octet-stream
Size: 26250 bytes
Desc: kill.diff
URL:
<http://rubyforge.org/pipermail/ironruby-core/attachments/20090109/ad193ad5/attachment-0001.obj>

Are any methods in RubyOps Exceptions #region (including those added by this
shelveset) emitted to IL? If not they should rather be in RubyUtils class.

On the other hand, SourceUnitTree.CheckForAsyncRaiseViaThreadAbortshould should
be marked as [Emitted], be in RubyOps.cs and
SourceUnitTree.GenerateCheckForAsyncException should use
Methods.CheckForAsyncRaiseViaThreadAbort to get the method.

Could we add a public helper that calls thread.Abort(new ThreadExitMarker());
move the private ThreadExitMarker class and IsRubyThreadExit to RubyUtils? Then
we won''t need to duplicate the marker class in ThreadOps?

Other than that, looks good.

Tomas

-----Original Message-----
From: Shri Borde 
Sent: Friday, January 09, 2009 3:33 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: Issues with Thread#kill and raising exception from ensure
clause

  tfpt review "/shelveset:kill;REDMOND\sborde"

  Comment  : 
  Added more tests for Thread#kill, and fixed bugs that were exposed. Throwing
an exception from the ensure clause exposes new code paths.
  
  Ruby allows Thread#kill to be completely subverted (not just deferred as an
infinite loop in an ensure clause would do) by raising an exception from a
ensure clause. I have not matched that behavior, and left it as a IronRuby bug
for now.

Inline...

-----Original Message-----
From: Tomas Matousek 
Sent: Friday, January 09, 2009 4:34 PM
To: Shri Borde; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Issues with Thread#kill and raising exception from
ensure clause

Are any methods in RubyOps Exceptions #region (including those added by this
shelveset) emitted to IL? If not they should rather be in RubyUtils class.
[Shri Borde] They are not emitted to IL. Moved it to RubyUtils...

On the other hand, SourceUnitTree.CheckForAsyncRaiseViaThreadAbortshould should
be marked as [Emitted], be in RubyOps.cs and
SourceUnitTree.GenerateCheckForAsyncException should use
Methods.CheckForAsyncRaiseViaThreadAbort to get the method.
[Shri Borde] Done

Could we add a public helper that calls thread.Abort(new ThreadExitMarker());
move the private ThreadExitMarker class and IsRubyThreadExit to RubyUtils? Then
we won''t need to duplicate the marker class in ThreadOps?
[Shri Borde] Its not duplicated. I already moved it out of ThreadOps.

Other than that, looks good.

Tomas

-----Original Message-----
From: Shri Borde 
Sent: Friday, January 09, 2009 3:33 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: Issues with Thread#kill and raising exception from ensure
clause

  tfpt review "/shelveset:kill;REDMOND\sborde"

  Comment  : 
  Added more tests for Thread#kill, and fixed bugs that were exposed. Throwing
an exception from the ensure clause exposes new code paths.
  
  Ruby allows Thread#kill to be completely subverted (not just deferred as an
infinite loop in an ensure clause would do) by raising an exception from a
ensure clause. I have not matched that behavior, and left it as a IronRuby bug
for now.