[Ironruby-core] Code Review: hangs in core\thread

Jim Deville jdeville at microsoft.com
Thu Jan 8 14:12:45 EST 2009


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
-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Tomas Matousek
Sent: Wednesday, January 07, 2009 2:37 PM
To: Shri Borde; ironruby-core at rubyforge.org; IronRuby External Code Reviewers
Subject: Re: [Ironruby-core] Code Review: hangs in core\thread

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

Tomas

-----Original Message-----
From: Shri Borde
Sent: Wednesday, January 07, 2009 2:18 PM
To: ironruby-core at rubyforge.org; IronRuby External Code Reviewers
Subject: RE: Code Review: hangs in core\thread

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

-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Shri Borde
Sent: Tuesday, January 06, 2009 11:49 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: [Ironruby-core] 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.


_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core



More information about the Ironruby-core mailing list