Skip to content

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 jviau and sharwell 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
Contributor

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
Contributor

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
Contributor

@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