Implement Queue#pop(non_block = false, timeout: nil)#6185
Conversation
092407b to
e56ae7e
Compare
| } | ||
|
|
||
| static inline rb_hrtime_t | ||
| rb_hrtime_sub(rb_hrtime_t a, rb_hrtime_t b) |
There was a problem hiding this comment.
I need rb_hrtime_sub to be able to compute the relative time from the absolute one. Since the sleep may be interrupted for other reasons and resumed, we pass around the final end time rather than a relative sleep time.
|
We need to investigate the way the fiber scheduler implementation handles timeouts, I suspect it will raise an exception, but it sounds like we should expand this definition to include "return nil" on timeout. Again, I suggested we have a consistent interface for this, e.g. |
eregon
left a comment
There was a problem hiding this comment.
Thank you for working on this, I was thinking I should try to make the time but I'm rather unfamiliar with the timeout stuff in CRuby.
| def test_queue_pop_timeout | ||
| q = Thread::Queue.new | ||
| t1 = Thread.new { q.pop(timeout: 1) } | ||
| assert_nil t1.join(2).value |
There was a problem hiding this comment.
This one test will take 1 second which is quite long, I'd use 0.1 or even better 0.001 to avoid wasting time during testing.
It would also be nice to test that it does return a pop'd value if there is one.
My bias: of course ruby/spec would be much better for this, and ensure other Ruby implementations have it sooner. For instance there is no way to run MRI tests of a more recent version, but that's supported by ruby/spec with PRETEND_RUBY_VERSION e.g. used in truffleruby, so e.g. this timeout could be added already in TruffleRuby even though it's not aiming for full 3.2 compatibility yet.
There was a problem hiding this comment.
This one test will take 1 second which is quite long, I'd use 0.1 or even better 0.001 to avoid wasting time during testing.
I agree, however the reason I test with an integer is that Integer and Float are two distinct codepaths.
My bias: of course ruby/spec would be much better for this
Yeah, I'll add some specs. I'd like this to be reviewed by someone familiar with the thread scheduler first (likely @ko1), there's no point writing specs if my code is flawed.
There was a problem hiding this comment.
there's no point writing specs if my code is flawed.
Just like tests it'd be extra confidence it works as expected, and it'd be useful regardless of the implementation :)
There was a problem hiding this comment.
I implemented SizedQueue#push and added some specs, let me know what you think.
There was a problem hiding this comment.
I removed SizedQueue#push since it was explicitly out of scope, I kept the code around, I'll open a second PR for it as soon as this one is merged.
There was a problem hiding this comment.
I [...] added some specs, let me know what you think.
Thanks! They look good.
e56ae7e to
05f4fda
Compare
6915d4c to
7cbc067
Compare
|
I talked with @ko1, he approved the implementation with two small modifications, I'm now waiting on his confirmation that the implementation matches the spec agreed on by Matz. |
[Feature #18774] As well as `SizedQueue#pop(timeout: sec)` If both `non_block=true` and `timeout:` are supplied, ArgumentError is raised.
7cbc067 to
a1936dc
Compare
[Feature #18774]
I created
thread_sync.rbfor easier handling of keyword arguments.