Skip to content
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 CancellationToken.CombineWith extension methods #346

Merged
merged 3 commits into from
Aug 8, 2018

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Aug 8, 2018

Using this can help reduce allocations in frequented code that links (for example) its own CancellationToken that tracks disposal with a caller-provided CancellationToken.

Allocations are avoided by only creating the linked CancellationTokenSource when there are actually multiple cancelable CancellationTokens (as opposed to the popular CancellationToken.None).

@AArnott AArnott added this to the v16.0 milestone Aug 8, 2018
@AArnott AArnott self-assigned this Aug 8, 2018
@AArnott AArnott requested review from sharwell and jviau August 8, 2018 14:58
/// Provides access to a <see cref="System.Threading.CancellationToken"/> that combines multiple other tokens,
/// and allows convenient disposal of any applicable <see cref="CancellationTokenSource"/>.
/// </summary>
public struct CombinedCancellationToken : IDisposable
Copy link
Member

Choose a reason for hiding this comment

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

💡 readonly struct

💡 Consider making this a top-level type. While it's vital users dispose of it properly, it seems reasonable that it would be stored for disposing at a later point.

Copy link
Member Author

@AArnott AArnott Aug 8, 2018

Choose a reason for hiding this comment

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

readonly struct

Done.

Consider making this a top-level type.

I think I see your point. It's not particularly the use case I'm going for though. Everywhere I've seen where multiple CancellationToken instances must be combined it's within a method and therefore only ever required as a local where var would suffice.
The cost of another top-level type is of course that it adds to completion lists and it's not a top-level scenario to engage with this type directly.
So I'm torn.

/// <param name="original">The first token.</param>
/// <param name="other">The second token.</param>
/// <returns>A struct that contains the combined <see cref="CancellationToken"/> and a means to release memory when you're done using it.</returns>
public static CombinedCancellationToken CombineWith(this CancellationToken original, CancellationToken other)
Copy link
Member

Choose a reason for hiding this comment

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

💡 Consider placing this in a new type CancellationTokenExtensions

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

/// <returns>A struct that contains the combined <see cref="CancellationToken"/> and a means to release memory when you're done using it.</returns>
public static CombinedCancellationToken CombineWith(this CancellationToken original, CancellationToken other)
{
if (original.IsCancellationRequested)
Copy link
Member

@sharwell sharwell Aug 8, 2018

Choose a reason for hiding this comment

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

💭 The alternative here is:

if (original.IsCancellationRequested || !other.CanBeCanceled)
{
  return new CombinedCancellationToken(original);
}

if (other.IsCancellationRequested || !original.CanBeCanceled)
{
  return new CombinedCancellationToken(other);
}

// This is the most expensive path to take since it involves allocating memory and requiring disposal.
// Before this point we've checked every condition that would allow us to avoid it.
return new CombinedCancellationToken(CancellationTokenSource.CreateLinkedTokenSource(original, other));

Not sure if the pair of these is simpler than the ternary at the end, but it seems to make the method "appear" less complex (shorter by ~5 lines with one less block).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can go with that.

@AArnott AArnott merged commit 8c70b2d into microsoft:master Aug 8, 2018
@AArnott AArnott deleted the CombineWith branch August 8, 2018 16:26
AArnott pushed a commit that referenced this pull request Feb 10, 2025
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants