Optimise the usage of CancellationTokens #481
Conversation
nathanwoctopusdeploy
requested review from
acodrington,
LukeButters and
sburmanoctopus
|
|
||
| public CancellationToken LinkedCancellationToken => LazyLinkedCancellationToken.Value; | ||
|
|
||
| Lazy<CancellationToken> LazyLinkedCancellationToken => new (() => |
There was a problem hiding this comment.
Removed the Lazy as the LinkedCancellationToken is always referenced so the Lazy is always created.
There was a problem hiding this comment.
Reason: Simplification to remove any possibility of retention due to the referenced objects of the lazy
| return CancellationToken.None; | ||
| LinkedCancellationToken = CancellationToken.None; | ||
| } | ||
| else if (InProgressRequestCancellationToken == CancellationToken.None) |
There was a problem hiding this comment.
Optimisations to not wrap a single cancelable cancellation token in a new cancellation token source
| @@ -38,7 +38,8 @@ public class SecureListener : IDisposable | |||
| readonly ILogFactory logFactory; | |||
| readonly Func<string> getFriendlyHtmlPageContent; | |||
| readonly Func<Dictionary<string, string>> getFriendlyHtmlPageHeaders; | |||
| readonly CancellationTokenSource cts = new(); | |||
| readonly CancellationTokenSource cts; | |||
| readonly CancellationToken cancellationToken; | |||
There was a problem hiding this comment.
Usage of a single cancellation token to avoid creating new cancellation tokens each time they are needed from the source. It's a simple pass-through object but we create a lot of them
There was a problem hiding this comment.
Oh so the CTS creates a new token each time it is asked for a token?
There was a problem hiding this comment.
Yes, it's a very lightweight struct that calls into the CancellationTokenSource, but it's another object with another reference.
| { | ||
| // The stream is wrapped in a NetworkTimeoutStream which handles closing the inner stream on timeout |
There was a problem hiding this comment.
For Async Halibut we don't need this weak ref and disposal as we wrap all Async Halibut stream operations so they cancel / timeout / close or dispose
There was a problem hiding this comment.
I had understood the cancellationToken is triggered in the dispose and this was a way to kill any streams when the secure listener is disposed.
Does this mean if the secure listener is disposed the accepted streams are not disposed?
There was a problem hiding this comment.
@LukeButters TcpClientManager is now Disposable and the SecureListener will Dispose of it
There was a problem hiding this comment.
Nice thanks! I think the result is simpler to follow code.
| var actionTask = action(linkedCancellationTokenSource.Token); | ||
| var cancellationTask = linkedCancellationTokenSource.Token.AsTask<T>(); |
There was a problem hiding this comment.
Removing the usage of .AsTask which does a .Register on a cancellation token in favour of a simpler flow using Task.Delay. The main driver is to remove the .Register
|
|
||
| namespace Halibut.Util | ||
| { | ||
| internal static class CancellationTokenExtensionMethods |
There was a problem hiding this comment.
No longer needed
nathanwoctopusdeploy
force-pushed
the
sast/cancellation-token-usage-changes
branch
from
db1df51
to
b906aad
Compare
| } | ||
| } | ||
|
|
||
| public CancellationToken ConnectingCancellationToken { get; set; } |
There was a problem hiding this comment.
A bit late to the party, but why are these two property setters public? Why not just read only properties?
Background
This PR has a few changes around the way cancellation tokens are used to reduce some undesirable memory growth and object retention observed in Async Halibut when running at scale.
The combination of these fixes seems to have reduced the observed memory growth, with the GC seen to be working more efficiently and has removed some suspect retention of object around Sockets.
Please see the comments for details of the individual changes.
Related to OctopusDeploy/Issues#8266
How to review this PR
Quality ✔️
Pre-requisites