Skip to content

Conversation

@kmahar
Copy link
Contributor

@kmahar kmahar commented Feb 14, 2020

This adds toArray to async MongoCursor and async ChangeStream.

*/
internal func all() -> EventLoopFuture<[T]> {
public func toArray() -> EventLoopFuture<[T]> {
guard self.isAlive else {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without this, we would never error even if you call this on a closed cursor and would always return an empty array, because self.cached.clear() would just return nil if the cursor is closed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. One thing to note is that calls to isAlive and cached.clear should probably both only occur while the cursor lock is held, which is making me think we may have to move the cache logic into the internal cursor after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the caching logic into the common cursor type, and added a static property on MongocCursorWrapper as we discussed to indicate whether we need to cache the first document or not

// iterating a dead cursor should error
expect(try cursor.next().wait()).to(throwError())

// iterating after calling all should error.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was moved down into the toArray test below

@kmahar kmahar requested a review from patrickfreed February 14, 2020 19:54
Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@kmahar kmahar merged commit ba8f3da into master Feb 18, 2020
@kmahar kmahar deleted the SWIFT-718/cursor-all branch February 18, 2020 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants