thr3ads.net - Ironruby core - [Ironruby-core] Code Review: hangs in core\thread [Jan 2009]

If this information is useful, please help other people find it:
Share via:

Shri Borde

2009-Jan-07 07:49 UTC

[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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: hangs.diff
Type: application/octet-stream
Size: 14893 bytes
Desc: hangs.diff
URL:
<http://rubyforge.org/pipermail/ironruby-core/attachments/20090106/da87d8e2/attachment-0001.obj>

Shri Borde

2009-Jan-07 22:17 UTC

head link

[Ironruby-core] 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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: hangs.diff
Type: application/octet-stream
Size: 26816 bytes
Desc: hangs.diff
URL:
<http://rubyforge.org/pipermail/ironruby-core/attachments/20090107/19b136cd/attachment-0001.obj>

Tomas Matousek

2009-Jan-07 22:36 UTC

head link

[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.

Jim Deville

2009-Jan-08 19:12 UTC

head link

[Ironruby-core] 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.
* 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

Shri Borde

2009-Jan-08 19:38 UTC

head link

[Ironruby-core] 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

Jim Deville

2009-Jan-08 19:42 UTC

head link

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

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

Ironruby core - Jan 2009 - Code Review: hangs in core\thread

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

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

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

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

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

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