Tests for concurrent queries. Finishes disposed iterators. #67
Conversation
| return success (key.some, data) | ||
|
|
||
| iter.next = next | ||
| iter.dispose = proc(): Future[?!void] {.async.} = |
There was a problem hiding this comment.
I think that disposing an inter should follow iter being finished, not the other way around.
There was a problem hiding this comment.
I'm sorry, I'm not sure what you mean. :o
There was a problem hiding this comment.
So setting finished to true, should be an internal to iterator event that's signaling that all items were yielded (line 197).
With this change, flag finished would be set to true whenever dispose() would be called (even if all items were not yielded). So as a client of this API you can finish iteration prematurely and call dispose(), but that's not the same as yielding all elements.
There was a problem hiding this comment.
🔴 Well the client won't even finish iteration because next will still happily return the next element as next doesn't check the finished flag to decide what to do. So if you're relying on it returning success (Key.none, ...) to know when to stop, this will not have the intended effect.
I suppose that to get the right semantics (assuming this is the right one) you need to check the state of the finished flag in addition to the state of the walker iterator (e.g if finished(walker) or iter.finished).
There was a problem hiding this comment.
I think I understand the desired behavior and I'm gonne write a test for it.
There was a problem hiding this comment.
Update: Finished iters will always return None when asked for next. There's a test to ensure this, and ensure that disposed iters become finished.
There was a problem hiding this comment.
Not sure why we're (incorrectly) changing the semantics of the iterators now?
as @tbekas is saying:
So setting finished to true, should be an internal to iterator event that's signaling that all items were yielded (line 197).
There was a problem hiding this comment.
I think the goal is to allow dispose to be called before the iterator returns all elements. I suppose this is useful because it's not always that you will want to consume the whole set of results from a query, and in that case you'll want to free up the resources (dispose) anyway. Is that the use case @benbierens?
There was a problem hiding this comment.
Yeah OK the concern I had is fixed by this.
There was a problem hiding this comment.
Yes, we can't assume the iterator will always run through all elements. I've added a test to show that you can properly clean up an iterator early by calling dispose on it. Any nexts would then yield nothing and the underlying resources can be closed.
gmega
left a comment
gmega
left a comment
There was a problem hiding this comment.
The tests are fine, but the semantics for disposal is a bit weird for fsds, wanna make sure that's what's intended.
| return success (key.some, data) | ||
|
|
||
| iter.next = next | ||
| iter.dispose = proc(): Future[?!void] {.async.} = |
There was a problem hiding this comment.
🔴 Well the client won't even finish iteration because next will still happily return the next element as next doesn't check the finished flag to decide what to do. So if you're relying on it returning success (Key.none, ...) to know when to stop, this will not have the intended effect.
I suppose that to get the right semantics (assuming this is the right one) you need to check the state of the finished flag in addition to the state of the walker iterator (e.g if finished(walker) or iter.finished).
gmega
left a comment
gmega
left a comment
There was a problem hiding this comment.
OK the concerns I had have been addressed so approving it.
| return success (key.some, data) | ||
|
|
||
| iter.next = next | ||
| iter.dispose = proc(): Future[?!void] {.async.} = |
There was a problem hiding this comment.
I think the goal is to allow dispose to be called before the iterator returns all elements. I suppose this is useful because it's not always that you will want to consume the whole set of results from a query, and in that case you'll want to free up the resources (dispose) anyway. Is that the use case @benbierens?
| return success (key.some, data) | ||
|
|
||
| iter.next = next | ||
| iter.dispose = proc(): Future[?!void] {.async.} = |
There was a problem hiding this comment.
Yeah OK the concern I had is fixed by this.
During debugging, I wrote these tests to check that the datastores can handle concurrent queries.
This didn't help. It didn't reveal any problems. But I figure we might as well keep the tests.