# Code Review: Thread.critical

**URL:** <https://www.ruby-forum.com/t/code-review-thread-critical/156953>\
**Category:** IronRuby\
**Created:** [January 5, 2009, 7:23pm UTC](https://www.ruby-forum.com/t/code-review-thread-critical/156953 "2009-01-05T19:23:22Z")\
**Posts on this page:** 4\
**Page:** 1

<div class="post-metadata">

**Author:** ![Shri\_B](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/s/898d66/32.png) [@Shri\_B](https://www.ruby-forum.com/u/Shri_B)\
**Post date:** [January 5, 2009, 7:23pm UTC](https://www.ruby-forum.com/t/code-review-thread-critical/156953/1 "2009-01-05T19:23:22Z")

</div>

tfpt review “/shelveset:critical;REDMOND\sborde”

Microsoft.Scripting.dll:

Fix to interpreter to get better stack trace even when call-site  
caching kicks in. The try-catch needs to be added to all code that can  
throw an exception in the interpreter (or preferably in one function  
like Interpreter.Interpret), but I have not done that here.

IronRuby:

Implements Thread.critical= by using Monitor.Enter/Exit. This will  
work for 90% or more of scenarios IMO. MRI does not schedule other  
threads while Thread.critical is true (while IronRuby will), but this is  
more an implementation detail that falls out with the green threads  
scheduler, than something that should be by spec. In fact, other threads  
do get scheduled anyway if the critical thread does Thread.pass etc  
(added tests to show this behavior if you want to play with it).  
Added tests  
Added irtests.bat to run all IronRuby tests with a single command, in  
four processes

---

<div class="post-metadata">

**Author:** ![Shri\_B](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/s/898d66/32.png) [@Shri\_B](https://www.ruby-forum.com/u/Shri_B)\
**Post date:** [January 5, 2009, 8:00pm UTC](https://www.ruby-forum.com/t/code-review-thread-critical/156953/2 "2009-01-05T20:00:58Z")

</div>

I’d rather see the functionality in irtest.bat in IronRuby’s run.bat.  
That way it runs with run 0.

The specs will probably need to be cleaned up to meet RubySpec style  
guidelines, and to get Brian to approve them. Go ahead and commit them,  
and I can clean them up, or we can work on them together later.

For future reference, style guidelines are here:  
[http://rubyspec.org/wiki/rubyspec/Style\_Guide](http://rubyspec.org/wiki/rubyspec/Style_Guide)

JD

---

<div class="post-metadata">

**Author:** ![Shri\_B](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/s/898d66/32.png) [@Shri\_B](https://www.ruby-forum.com/u/Shri_B)\
**Post date:** [January 5, 2009, 10:47pm UTC](https://www.ruby-forum.com/t/code-review-thread-critical/156953/3 "2009-01-05T22:47:54Z")

</div>

The implementation of ThreadOps.\_CriticalMonitor and  
ThreadOps.\_IsInCriticalRegion as class-level fields will cause state  
information to leak between ScriptRuntimes. These should probably be  
put on the RubyContext, which can either be done directly or by using  
RubyContext.GetOrCreateLibraryData to create it dynamically.

Looks good otherwise.

---

<div class="post-metadata">

**Author:** ![Shri\_B](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/s/898d66/32.png) [@Shri\_B](https://www.ruby-forum.com/u/Shri_B)\
**Post date:** [January 6, 2009, 7:05am UTC](https://www.ruby-forum.com/t/code-review-thread-critical/156953/4 "2009-01-06T07:05:04Z")

</div>

Good catch. I have fixed the issue. Also updated the tests according to  
Jim’s suggestions. Also, the last test using Thread.kill was wrong but  
seemed to succeed since “flunk” fails silently when used on the non-main  
thread. New patch included if you want to take a look. Will go ahead and  
submit to Snap.