Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Add WithCancellation for async enumerables #21939

Merged
merged 1 commit into from
Jan 11, 2019

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Jan 10, 2019

Contributes to https://github.com/dotnet/corefx/issues/33909
cc: @jcouv, @kouvel, @tarekgh

Relates to async-streams umbrella: dotnet/roslyn#24037

{
/// <summary>Provides an awaitable async enumerable that enables cancelable iteration and configured awaits.</summary>
[StructLayout(LayoutKind.Auto)]
public readonly struct ConfiguredCancelableAsyncEnumerable<T>
Copy link
Member

@jcouv jcouv Jan 10, 2019

Choose a reason for hiding this comment

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

ConfiguredCancelableAsyncEnumerable [](start = 27, length = 35)

nit: I'm unsure which spelling is more common (cancellable vs. cancelable). I would have gone with two L's.

Apparently, US is one L, and UK is two L's ;-)
https://www.collinsdictionary.com/dictionary/english/cancellable

But I'd still lean towards two L's, to match CancellationToken

Copy link
Member

Choose a reason for hiding this comment

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

We have it one L in other places like TaskCanceledException, Task.IsCanceled, Task.FromCanceled, TaskCompletionSource.SetCanceled..etc.

Although we are not consistent in the spelling as CancellationToken uses 2 L's but it looks the majority of the usage is using 1 L.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, we have some inconsistency from early usage of "cancel" in .NET Framework 3.x and earlier. As of .NET Framework 4, we settled on cancel, canceled, canceling, canceler, cancelable, etc., all with one L, and then cancellation with two Ls.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM
The spelling question probably doesn't matter much as most people will not see this type. They will see WithCancellation and CancellationToken.
I don't know how testing works. I assume you'll have some coverage in another PR/repo?

@tarekgh
Copy link
Member

tarekgh commented Jan 10, 2019

I don't know how testing works. I assume you'll have some coverage in another PR/repo?

@jcouv you already included in the test changes PR dotnet/corefx#34528

@stephentoub
Copy link
Member Author

@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test please

@stephentoub stephentoub merged commit b46881a into dotnet:master Jan 11, 2019
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Jan 11, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/mono that referenced this pull request Jan 11, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Jan 11, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
MichalStrehovsky pushed a commit to dotnet/corert that referenced this pull request Jan 11, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
marek-safar pushed a commit to mono/mono that referenced this pull request Jan 11, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
stephentoub added a commit to dotnet/corefx that referenced this pull request Jan 11, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
baulig pushed a commit to mono/corefx that referenced this pull request Feb 5, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
(cherry picked from commit a938f91)
baulig pushed a commit to mono/corefx that referenced this pull request Feb 5, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
(cherry picked from commit a938f91)
@clairernovotny
Copy link
Member

clairernovotny commented Feb 18, 2019

This PR is a bit problematic when trying to copy these types into a downlevel platform and support unification with type forwarders. The problem is twofold:

  1. The internal constructors. If I copy the type, I can't correctly type forward since the new type's ctor won't be visible.
  2. Adding methods to TaskExtensions. I can't forward certain methods; it would be much easier to add those to a new type that can be copied in full and typeforwarded on the netstandard2.1/netcoreapp3.0 version.

Can the ctor's be made public and the new methods from TaskExtension be split into its own type?

This is mainly because of 2, since I'd have to carry the method bodies in the 3.0 target, even if hidden by a reference assembly, and those implementation's can't create the instance of the struct.

I think the ctor visibility issue is moot if the two extension methods are split into a new type. Then I can copy the type and static class in full, and elide them in place with type forwarders on the 3.0 target.

/cc @bartdesmet @terrajobst

@stephentoub stephentoub deleted the withcancellation branch February 19, 2019 03:19
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants