Code Review: hangs in core\thread

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.

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

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

Tomas

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

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

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

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs