-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-3458: Extend IAsyncCursor and IAsyncCursorSource to support IAsyncEnumerable #1708
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
base: main
Are you sure you want to change the base?
Changes from all commits
d33d623
9998b8b
eff826b
7b62225
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,12 @@ | |
using System.Collections; | ||
using System.Collections.Generic; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using MongoDB.Driver.Core.Misc; | ||
|
||
namespace MongoDB.Driver.Core.Operations | ||
{ | ||
internal class AsyncCursorEnumerator<TDocument> : IEnumerator<TDocument> | ||
internal class AsyncCursorEnumerator<TDocument> : IEnumerator<TDocument>, IAsyncEnumerator<TDocument> | ||
{ | ||
// private fields | ||
private IEnumerator<TDocument> _batchEnumerator; | ||
|
@@ -72,6 +73,12 @@ public void Dispose() | |
} | ||
} | ||
|
||
public ValueTask DisposeAsync() | ||
{ | ||
Dispose(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add a comment like:
|
||
return default; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My first reaction was that It took me some research to figure out why you were returning I suggest the following:
It's a little more verbose, but it documents why we sometimes have to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a bug definitely. This is documented behavior:
https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.valuetask?view=net-9.0#remarks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't say it was a bug. I meant it "looked" like a bug. I'm saying that we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't say it wasn't technically the same. I'm saying that we should use ValueTask.CompletedTask when available. It's more readable than |
||
} | ||
|
||
public bool MoveNext() | ||
{ | ||
ThrowIfDisposed(); | ||
|
@@ -82,24 +89,46 @@ public bool MoveNext() | |
return true; | ||
} | ||
|
||
while (true) | ||
while (_cursor.MoveNext(_cancellationToken)) | ||
{ | ||
if (_cursor.MoveNext(_cancellationToken)) | ||
_batchEnumerator?.Dispose(); | ||
_batchEnumerator = _cursor.Current.GetEnumerator(); | ||
if (_batchEnumerator.MoveNext()) | ||
{ | ||
_batchEnumerator?.Dispose(); | ||
_batchEnumerator = _cursor.Current.GetEnumerator(); | ||
if (_batchEnumerator.MoveNext()) | ||
{ | ||
return true; | ||
} | ||
return true; | ||
} | ||
else | ||
} | ||
|
||
_batchEnumerator?.Dispose(); | ||
_batchEnumerator = null; | ||
_finished = true; | ||
return false; | ||
} | ||
|
||
public async ValueTask<bool> MoveNextAsync() | ||
{ | ||
ThrowIfDisposed(); | ||
_started = true; | ||
|
||
if (_batchEnumerator != null && _batchEnumerator.MoveNext()) | ||
{ | ||
return true; | ||
} | ||
|
||
while (await _cursor.MoveNextAsync(_cancellationToken).ConfigureAwait(false)) | ||
{ | ||
_batchEnumerator?.Dispose(); | ||
_batchEnumerator = _cursor.Current.GetEnumerator(); | ||
if (_batchEnumerator.MoveNext()) | ||
{ | ||
_batchEnumerator = null; | ||
_finished = true; | ||
return false; | ||
return true; | ||
} | ||
} | ||
|
||
_batchEnumerator?.Dispose(); | ||
_batchEnumerator = null; | ||
sanych-sun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_finished = true; | ||
return false; | ||
} | ||
|
||
public void Reset() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,27 +13,37 @@ | |
* limitations under the License. | ||
*/ | ||
|
||
using System; | ||
using System.Collections; | ||
using System.Collections.Generic; | ||
using System.Threading; | ||
using MongoDB.Driver.Core.Misc; | ||
|
||
namespace MongoDB.Driver.Core.Operations | ||
{ | ||
internal class AsyncCursorSourceEnumerableAdapter<TDocument> : IEnumerable<TDocument> | ||
internal class AsyncCursorSourceEnumerableAdapter<TDocument> : IEnumerable<TDocument>, IAsyncEnumerable<TDocument> | ||
{ | ||
// private fields | ||
private readonly CancellationToken _cancellationToken; | ||
private readonly IAsyncCursorSource<TDocument> _source; | ||
|
||
// constructors | ||
public AsyncCursorSourceEnumerableAdapter(IAsyncCursorSource<TDocument> source) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest to call another constructor here, like: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would not have added this constructor. It makes it too easy for the caller to forget to pass in the cancellationToken. Let the caller provide |
||
: this(source, CancellationToken.None) | ||
{ | ||
} | ||
|
||
public AsyncCursorSourceEnumerableAdapter(IAsyncCursorSource<TDocument> source, CancellationToken cancellationToken) | ||
{ | ||
_source = Ensure.IsNotNull(source, nameof(source)); | ||
_cancellationToken = cancellationToken; | ||
} | ||
|
||
public IAsyncEnumerator<TDocument> GetAsyncEnumerator(CancellationToken cancellationToken = default) | ||
{ | ||
var cursor = _source.ToCursor(cancellationToken); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling Presumable the caller did not want to execute the query synchronously. Can we defer query execution until Thoughts? |
||
return new AsyncCursorEnumerator<TDocument>(cursor, cancellationToken); | ||
} | ||
|
||
// public methods | ||
public IEnumerator<TDocument> GetEnumerator() | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not have added this constructor.
It makes it too easy for the caller to forget to pass in the cancellationToken.
Let the caller provide
CancellationToken.None
if it needs to.