Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SizedQueue#num_waiting regression #3597

Merged
merged 1 commit into from Jan 19, 2016

Conversation

Projects
None yet
5 participants
@etehtsea
Copy link
Contributor

etehtsea commented Jan 12, 2016

Introduced in d13405b.

JRuby 1.7 and MRI 2.2.3:

>> sq = SizedQueue.new(1)
=> #<SizedQueue:0x61f8bee4>
>> sq.push(1)
=> nil
>> Thread.new { sq.push(2) }
=> #<Thread:0x7fac631b run>
>> sq.num_waiting
=> 1

JRuby 9.0.4.0:

>> sq = SizedQueue.new(1)
=> #<SizedQueue:0x103f852>
>> sq.push(1)
=> #<SizedQueue:0x103f852>
>> Thread.new { sq.push(2) }
=> #<Thread:0x4ae82894 sleep>
>> sq.num_waiting
=> 0

P.S. I'm not specialized in Java, but it seems to work.

@etehtsea etehtsea changed the title Add spec for SizedQueue#num_waiting regression Fix SizedQueue#num_waiting regression Jan 12, 2016

@etehtsea etehtsea force-pushed the etehtsea:num_waiting_regression branch from 7824b91 to 892592b Jan 12, 2016

@eregon

This comment has been minimized.

Copy link
Member

eregon commented Jan 12, 2016

Thanks for the nice spec!

@thedarkone

This comment has been minimized.

Copy link
Contributor

thedarkone commented Jan 14, 2016

I think this is also fixed on ruby-2.3 branch (with the new queue).

@etehtsea

This comment has been minimized.

Copy link
Contributor Author

etehtsea commented Jan 19, 2016

@thedarkone

~/Projects/jruby [ ruby-2.3: ✗ ] 7d ⚡ bin/jruby spec/mspec/bin/mspec run spec/ruby/library/thread/sizedqueue/num_waiting_spec.rb                                  2.2.3 ☺
jruby 9.0.5.0-SNAPSHOT (2.3.0) 2016-01-19 badd94b Java HotSpot(TM) 64-Bit Server VM 25.60-b23 on 1.8.0_60-b27 [darwin-x86_64]

1)
Thread::SizedQueue#num_waiting reports the number of threads waiting to push FAILED
Expected 0
 to equal 1

/Users/kes/Projects/jruby/spec/ruby/library/thread/sizedqueue/num_waiting_spec.rb:12:in `block in (root)'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyEnumerable.java:1562:in `all?'
org/jruby/RubyFixnum.java:296:in `times'
org/jruby/RubyArray.java:1555:in `each'
/Users/kes/Projects/jruby/spec/ruby/library/thread/sizedqueue/num_waiting_spec.rb:5:in `<top>'
org/jruby/RubyKernel.java:957:in `load'
org/jruby/RubyBasicObject.java:1633:in `instance_eval'
org/jruby/RubyArray.java:1555:in `each'
[\ | ==================100%================== | 00:00:00]      1F      0E

Finished in 0.015000 seconds

1 file, 2 examples, 6 expectations, 1 failure, 0 errors, 0 tagged

Spec failed but I haven't made any further investigations yet.

UPD: Seems that failure is related to race-condition issue. When I added sleep 1 spec passed.

@kares kares added this to the JRuby 9.0.5.0 milestone Jan 19, 2016

Konstantin Shabanov

@etehtsea etehtsea force-pushed the etehtsea:num_waiting_regression branch from 892592b to d54e4cd Jan 19, 2016

@etehtsea

This comment has been minimized.

Copy link
Contributor Author

etehtsea commented Jan 19, 2016

I updated spec.

P.S. Hope it's safe to wait until thread fall asleep.

@kares

This comment has been minimized.

Copy link
Member

kares commented Jan 19, 2016

@thedarkone should be good to merge as is for JRuby 9.0.5 (non ruby-2.3), right?

@headius

This comment has been minimized.

Copy link
Member

headius commented Jan 19, 2016

I'll look at merging this today.

@headius

This comment has been minimized.

Copy link
Member

headius commented Jan 19, 2016

Note that https://github.com/spec/ruby has a different shared spec for num_waiting now, and JRuby's ruby-2.3 branch passes it fine: https://github.com/ruby/spec/blob/master/library/thread/shared/queue/num_waiting.rb

I'll merge this into master for 9.0.5.0 but I'll leave it up to you folks to decide which spec is better.

@headius headius merged commit d54e4cd into jruby:master Jan 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@etehtsea

This comment has been minimized.

Copy link
Contributor Author

etehtsea commented Jan 19, 2016

@etehtsea etehtsea deleted the etehtsea:num_waiting_regression branch Jan 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.