Ensuring cancellation of tokens when using GetLinkedCancellationToken(a,b) #55968
Conversation
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
|
@MattyLeslie thanks for the contribution.
We don't see any updated tests are they missing? |
|
@javiercn Sorry I forgot to push! |
dotnet-policy-service
bot
added
the
pending-ci-rerun
|
@javiercn, may I please request a review for this. |
1 similar comment
|
@javiercn, may I please request a review for this. |
|
@MattyLeslie sorry, this wasn't on my radar. We'll take a look by end of week. |
There was a problem hiding this comment.
Thanks for the contribution, @MattyLeslie, and apologies for the delayed review.
I've made some suggestions that will need to be addressed before this can get merged. If you'd like me to handle those, I'm happy to do so!
| using (var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(_streamCancellationToken, cancellationToken)) | ||
| { | ||
| return await _pipeReaderStream.ReadAsync(buffer.AsMemory(offset, count), linkedCts.Token); | ||
| } |
There was a problem hiding this comment.
A nice characteristic of the previous implementation was that it didn't allocate a new CancellationTokenSource if at least one CancellationToken was not cancelable. It would be good to retain that characteristic if possible.
| Assert.True(cts.Token.CanBeCanceled); | ||
| Assert.False(cts.IsCancellationRequested); |
There was a problem hiding this comment.
Does this validate disposal of the linked CTS created in the ReadAsync() method? These asserts are acting on the CTS created in this test, which shouldn't be affected by any linked CTS that gets created from it.
Same question applies for the other test.
|
@MackinnonBuck If you're will to carry it over the line that would be great otherwise I'll address that feedback as soon as I get time. |
…"ReadAsync_ByteArray_DisposesCancellationTokenSource"
MackinnonBuck
force-pushed
the
Ensuring-cancellation-of-tokens-when-using-GetLinkedCancellationToken(a,b)
branch
2 times, most recently
from
d7af27e to
708c0fd
Compare
MackinnonBuck
force-pushed
the
Ensuring-cancellation-of-tokens-when-using-GetLinkedCancellationToken(a,b)
branch
from
1cd9b14 to
844215d
Compare
| if (a.CanBeCanceled && b.CanBeCanceled) | ||
| { | ||
| return CancellationTokenSource.CreateLinkedTokenSource(a, b).Token; | ||
| } | ||
| else if (a.CanBeCanceled) | ||
| { | ||
| return a; | ||
| } |
There was a problem hiding this comment.
What's the reason for this logic disappearing and relying directly on ValueLinkdedCancellationTokenSource.Create
There was a problem hiding this comment.
Nevermind, I saw it got moved. I thought ValueLinkdedCancellationTokenSource was a framework class.
…(a,b) (#55968) Co-authored-by: Mackinnon Buck <[email protected]>
3 participants
Fix HANDLE_LEAK issue by properly disposing
CancellationTokenSourceinRemoteJSDataStreamSummary of the changes
Properly dispose
CancellationTokenSourceinRemoteJSDataStreamto prevent handle leaks.Description
This PR addresses the HANDLE_LEAK issue identified in
RemoteJSDataStreamby ensuring thatCancellationTokenSourceinstances are properly disposed of. TheGetLinkedCancellationTokenmethod was removed, and using statements were added directly within theReadAsyncmethods to manage the lifecycle ofCancellationTokenSource. Additionally, the test cases were updated to validate the correct disposal ofCancellationTokenSource.Changes Made
GetLinkedCancellationTokenmethod.ReadAsyncmethods for proper disposal ofCancellationTokenSource.CancellationTokenSource.Fixes #50676