Code Review: hangs in core\thread

Ruby IronRuby
1

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.

2

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

3

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

Tomas

4

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

5

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

6

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