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.

Sign up for GitHub

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

Jump to bottom

Fix SizedQueue#num_waiting regression #3597

Merged
merged 1 commit into from
Jan 19, 2016
Merged

Fix SizedQueue#num_waiting regression #3597

merged 1 commit into from
Jan 19, 2016
+27 −1

Conversation

etehtsea
Copy link
Contributor

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 num_waiting_regression branch from 7824b91 to 892592b Compare January 12, 2016 12:31
@eregon
Copy link
Member

eregon commented Jan 12, 2016

Thanks for the nice spec!

@thedarkone
Copy link
Contributor

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

@etehtsea
Copy link
Contributor Author

@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
@etehtsea etehtsea force-pushed the num_waiting_regression branch from 892592b to d54e4cd Compare January 19, 2016 09:25
@etehtsea
Copy link
Contributor Author

I updated spec.

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

@kares
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
Copy link
Member

headius commented Jan 19, 2016

I'll look at merging this today.

@headius
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
@etehtsea
Copy link
Contributor Author

@headius I don't see any difference from existing shared spec https://github.com/jruby/jruby/blob/master/spec/ruby/library/thread/shared/queue/num_waiting.rb

@etehtsea etehtsea deleted the num_waiting_regression branch January 19, 2016 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
JRuby 9.0.5.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@etehtsea @eregon @thedarkone @kares @headius