CSHARP-3458: Extend IAsyncCursor and IAsyncCursorSource to support IAsyncEnumerable #1708
Conversation
adelinowona
requested review from
rstam and
BorisDog
and removed request for
sanych-sun
|
As far as I know |
| { | ||
| // private fields | ||
| private readonly CancellationToken _cancellationToken; | ||
| private readonly IAsyncCursorSource<TDocument> _source; | ||
|
|
||
| // constructors | ||
| public AsyncCursorSourceEnumerableAdapter(IAsyncCursorSource<TDocument> source) |
There was a problem hiding this comment.
I would suggest to call another constructor here, like:
: this(source, CancellationToken.None)
There was a problem hiding this comment.
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.
| { | ||
| private readonly CancellationToken _cancellationToken; | ||
| private readonly IAsyncCursor<TDocument> _cursor; | ||
| private bool _hasBeenEnumerated; | ||
|
|
||
| public AsyncCursorEnumerableOneTimeAdapter(IAsyncCursor<TDocument> cursor) |
There was a problem hiding this comment.
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.
| { | ||
| // private fields | ||
| private readonly CancellationToken _cancellationToken; | ||
| private readonly IAsyncCursorSource<TDocument> _source; | ||
|
|
||
| // constructors | ||
| public AsyncCursorSourceEnumerableAdapter(IAsyncCursorSource<TDocument> source) |
There was a problem hiding this comment.
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.
| @@ -72,6 +73,12 @@ public void Dispose() | |||
| } | |||
| } | |||
|
|
|||
| public ValueTask DisposeAsync() | |||
| { | |||
| Dispose(); | |||
There was a problem hiding this comment.
I would add a comment like:
Dispose(); // TODO: implement true async disposal
| public ValueTask DisposeAsync() | ||
| { | ||
| Dispose(); | ||
| return default; |
There was a problem hiding this comment.
My first reaction was that default was a bug, that we should be returning ValueTask.CompletedTask.
It took me some research to figure out why you were returning default.
I suggest the following:
#if NET6_0_OR_GREATER
return ValueTask.CompletedTask;
#else
return default; // prior to NET6_0 you have to fake ValueTask.CompletedTask using default
#endif
It's a little more verbose, but it documents why we sometimes have to use default instead of ValueTask.CompletedTask, and when we remove support for netstandard2.1 this will get cleaned up.
There was a problem hiding this comment.
It's not a bug definitely. This is documented behavior:
An instance created with the parameterless constructor or by the default(ValueTask) syntax (a zero-initialized structure) represents a synchronously, successfully completed operation.
https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.valuetask?view=net-9.0#remarks
There was a problem hiding this comment.
I didn't say it was a bug. I meant it "looked" like a bug.
I'm saying that we should use ValueTask.CompletedTask when available.
There was a problem hiding this comment.
Yep, but it's technically the same.
ValueTask.CompletedTask is more readable though.
There was a problem hiding this comment.
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 default (who wants to go hunt down the documentation to figure out what default means).
| 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.
Calling _source.ToCursor is a BLOCKING call that executes the query.
Presumable the caller did not want to execute the query synchronously.
Can we defer query execution until asyncEnumerator.MoveNextAsync() is called?
Thoughts?
| public ValueTask DisposeAsync() | ||
| { | ||
| Dispose(); | ||
| return default; |
There was a problem hiding this comment.
I didn't say it was a bug. I meant it "looked" like a bug.
I'm saying that we should use ValueTask.CompletedTask when available.
| public ValueTask DisposeAsync() | ||
| { | ||
| Dispose(); | ||
| return default; |
There was a problem hiding this comment.
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 default (who wants to go hunt down the documentation to figure out what default means).
No description provided.