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

Jim Deville jdeville at microsoft.com
Thu Jan 8 14:42:17 EST 2009


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


-----Original Message-----
From: Shri Borde
Sent: Thursday, January 08, 2009 11:39 AM
To: Jim Deville; ironruby-core at rubyforge.org; IronRuby External Code Reviewers
Subject: RE: Code Review: hangs in core\thread

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


-----Original Message-----
From: Jim Deville
Sent: Thursday, January 08, 2009 11:13 AM
To: ironruby-core at rubyforge.org; Shri Borde; IronRuby External Code Reviewers
Subject: RE: Code Review: hangs in core\thread

Test Review:
classes.rb:
* can you change the get_status_... methods to status_... methods? The extra get isn't really used in Ruby.
[Shri Borde] Done


* Did you want to have possibly stale information in the Status class?
[Shri Borde] What do you mean by this? It should capture the state of the thread whenever Status.new was called.

* 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
[Shri Borde] Done


* 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.
[Shri Borde] Done

* Can you modify method names and variables to be snake_case instead of CamelCase?
[Shri Borde] Done

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?
[Shri Borde] I copied it from inspect_spec.rb. Until we understand how all the guards are supposed to work (you are following up about that), I don't know how to clean this up. So I keeping the status quo.

Critical_spec.rb:
* add a before(:each) block and move the ScratchPad.clear to there, then you can add ScratchPad without thinking about it.
[Shri Borde] Done. I think MSpec should be doing this, but oh well.

* Extra comment (#end) on line 49
[Shri Borde] Removed

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.
[Shri Borde] Removed it. I had already added a separate test for Thread#value. No point having the same test in two places.

Raise_spec.rb:
* You might need to remove the " raises an exception on running thread" tag. I think I added one when doing dates.
[Shri Borde] Done

Wakeup.rb:
* Line 38 has an unused Channel
[Shri Borde] Removed

* 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.
[Shri Borde] This test existed in wakeup_spec.rb and you said was removed from RubySpec. I am actually tempted to remove it completely. I will check it in as is and will let you figure out what to do when you pull in the new RubySpecs.


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