Forum: IronRuby Code Review: hangs in core\thread

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
Shri B. (Guest)
on 2009-01-07 10:11
(Received via mailing list)
Attachment: hangs.diff (0 Bytes)
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.
Shri B. (Guest)
on 2009-01-08 00:28
(Received via mailing list)
Attachment: hangs.diff (0 Bytes)
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
Tomas M. (Guest)
on 2009-01-08 01:04
(Received via mailing list)
I would prefer using a field rather than a private auto-property for
IsSleeping.
Other than that, changes in ThreadOps look good.

Tomas
Jim D. (Guest)
on 2009-01-08 21:24
(Received via mailing list)
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
Shri B. (Guest)
on 2009-01-08 21:53
(Received via mailing list)
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
Jim D. (Guest)
on 2009-01-08 21:55
(Received via mailing list)
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
This topic is locked and can not be replied to.