# Code Review: hangs in core\\thread

**URL:** <https://www.ruby-forum.com/t/code-review-hangs-in-core-thread/157087>\
**Category:** IronRuby\
**Created:** [January 7, 2009, 9:11am UTC](https://www.ruby-forum.com/t/code-review-hangs-in-core-thread/157087 "2009-01-07T09:11:53Z")\
**Posts on this page:** 6\
**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 7, 2009, 9:11am UTC](https://www.ruby-forum.com/t/code-review-hangs-in-core-thread/157087/1 "2009-01-07T09:11:53Z")

</div>

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

Comment :  
Non-deterministic fixes. I ran all the core\thread specs in a loop in  
multiple processes and ran into a few non-deterministic failures. This  
change fixes everything I ran into. The only product change is that  
Thread#wakeup is not queued up if the target thread is not sleeping.  
This is the MRI behavior.

Also started using comments in the tag files like this - fails(“reason  
why”):test name. I will undo this if Jim says that he will be  
regenerating the tags file periodically.

---

<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 7, 2009, 11:28pm UTC](https://www.ruby-forum.com/t/code-review-hangs-in-core-thread/157087/2 "2009-01-07T23:28:01Z")

</div>

Since no one has reviewed this yet, I have updated the change to factor  
out the tests for alive?, inspect, status and stop?

Another product change was that Thread#value should return false if the  
thread was killed. Only a thread with an uncaught exception should  
return nil.

Thanks,  
Shri

---

<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 8, 2009, 12:04am UTC](https://www.ruby-forum.com/t/code-review-hangs-in-core-thread/157087/3 "2009-01-08T00:04:59Z")

</div>

I would prefer using a field rather than a private auto-property for  
IsSleeping.  
Other than that, changes in ThreadOps look good.

Tomas

---

<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 8, 2009, 8:24pm UTC](https://www.ruby-forum.com/t/code-review-hangs-in-core-thread/157087/4 "2009-01-08T20:24:33Z")

</div>

Test Review:  
classes.rb:

- can you change the get\_status\_… methods to status\_… methods? The  
extra get isn’t really used in Ruby.
- Did you want to have possibly stale information in the Status class?
- for running thread, why not just do:  
def status\_of\_running\_thread  
t = Thread.new {loop {}}  
Thread.pass until t.status == “run”  
status = Status.new t  
t.kill  
status  
end
- critical\_should\_be\_false should use ScratchPad instead of should’s in  
the method. I would suggest trying to always keep shoulds in the spec  
itself.
- Can you modify method names and variables to be snake\_case instead of  
CamelCase?

Alive\_spec.rb:

- (new thing I’m going to start doing with new guards) What’s the  
justification for the compliant\_on(:ruby) guard. Why isn’t it a tag?

Critical\_spec.rb:

- add a before(:each) block and move the ScratchPad.clear to there, then  
you can add ScratchPad without thinking about it.
- Extra comment (#end) on line 49

Exit.rb:

- lines 11 and 23: I don’t see a reason for checking the value of the  
threads. You are already checking the ScratchPad, so you know the  
execution ran.

Raise\_spec.rb:

- You might need to remove the " raises an exception on running thread"  
tag. I think I added one when doing dates.

Wakeup.rb:

- Line 38 has an unused Channel
- 1000 times for the deadlock test might be too long for a test. In  
principle it’s nice for the stress portion, but it seems too long. I’d  
also like to find another way than 1.should == 1, but you can check that  
in and I’ll find a way.

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 8, 2009, 8:55pm UTC](https://www.ruby-forum.com/t/code-review-hangs-in-core-thread/157087/5 "2009-01-08T20:55:07Z")

</div>

I meant that the status of the thread might change after you call  
Status.new, however, it looks like you meant for that to happen:

" It should capture the state of the thread whenever Status.new was  
called."

I just wanted to be 100% sure this was the case. Everything else looks  
good.

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 8, 2009, 8:53pm UTC](https://www.ruby-forum.com/t/code-review-hangs-in-core-thread/157087/6 "2009-01-08T20:53:55Z")

</div>

Inline…

Everything has been addressesd except I am not sure what you meant by "  
Did you want to have possibly stale information in the Status class? ".  
The shelveset has been updated.

Thanks,  
Shri