-
Notifications
You must be signed in to change notification settings - Fork 164
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
Add new subscribe API #3403
Add new subscribe API #3403
Conversation
Pull Request Test Coverage Report for Build 5736508132
💛 - Coveralls |
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.
Looks good.
By the way, it was a little bit difficult to see the body of SubscribeAsync
together with the removal of the namespace block 😁
@@ -1,11 +1,23 @@ | |||
## vNext (TBD) | |||
|
|||
### Enhancements | |||
* None | |||
* Added `IQueryable.SubscribeAsync` API as a shorthand for using `SubscriptionSet.Add`. It is a syntax sugar that roughly translates to: |
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 add the equivalent code that uses SubscribeAsync
, so it's a little bit clearer
{ | ||
var tcs = new TaskCompletionSource(); | ||
if (cancellationToken?.IsCancellationRequested == true) | ||
{ | ||
tcs.TrySetCanceled(cancellationToken.Value); |
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 am not sure I understand why we need this and the following line. Can we just return?
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.
If cancellation is requested, the expected behavior would be to throw a TaskCancelledException
, even if we didn't start doing the work. Regarding why we're cancelling the tcs vs throwing a TaskCancelledException
, I guess both are roughly the same and don't have strong feelings about one or the other.
That's why you're supposed to read the PR description 😜 |
Description
Fixes RNET-865
Adds
IQueryable.SubscribeAsync
syntax sugar.Adds
cancellationToken
argument toSession.WaitForDownloadAsync
,Session.WaitForUploadAsync
, andSubscriptionSet.WaitForSynchronization
. This doesn't cancel the underlying Core wait operation, just the managed task. While it's not ideal, the native wait operation is not really expensive and it will eventually complete anyway, so we are not leaking resources.There are some whitespace changes, so it might be a good idea to review with the "Ignore whitespace" option.
TODO