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

Remove CancelAfter and Delay from internal TimeProvider #1348

Closed
martincostello opened this issue Jun 24, 2023 · 3 comments · Fixed by #1355
Closed

Remove CancelAfter and Delay from internal TimeProvider #1348

martincostello opened this issue Jun 24, 2023 · 3 comments · Fixed by #1355
Labels
v8 Issues related to the new version 8 of the Polly library.
Milestone

Comments

@martincostello
Copy link
Member

Attempting to rebase #1144 for .NET 8 and to consume TimeProvider from NuGet (and use the new FakeTimeProvider implementation) things get complicated because there are various tests that rely on TimeProvider having a virtual CancelAfter() and Delay() method that can be overridden in the tests.

// This one is not on TimeProvider, temporarly we need to use it
public virtual Task Delay(TimeSpan delay, CancellationToken cancellationToken = default) => Task.Delay(delay, cancellationToken);
// This one is not on TimeProvider, temporarly we need to use it
public virtual void CancelAfter(CancellationTokenSource source, TimeSpan delay) => source.CancelAfter(delay);

The real implementation instead has extension methods (which we can't mock) which are TimeProvider.Delay() and TimeProvider.CreateCancellationTokenSource().

To make the switch over for .NET 8 simpler, we should sync the current internal implementation to be source compatible with the .NET 8 rather than rely on methods which won't exist. That way, the tests will not need to change much and the switch over should be a lot cleaner.

@martincostello martincostello added the v8 Issues related to the new version 8 of the Polly library. label Jun 24, 2023
@martincostello martincostello added this to the v8.0.0 milestone Jun 24, 2023
@martincostello
Copy link
Member Author

martincostello commented Jun 24, 2023

Bonus of trying to use use FakeTimeProvider in our tests is that I found this bug: dotnet/extensions#4114

@martintmk
Copy link
Contributor

Thanks for reporting this. We should definitely sync the implementations to reduce the size of #1144 PR.

Our TimeProvider should include the CreateTimer method:
https://github.com/dotnet/runtime/blob/cfe30c057debbdd9ad21b36ae61062dbe72a1f83/src/libraries/Common/src/System/TimeProvider.cs#L156

We should also port the extensions:
https://github.com/dotnet/runtime/blob/cfe30c057debbdd9ad21b36ae61062dbe72a1f83/src/libraries/Microsoft.Bcl.TimeProvider/src/System/Threading/Tasks/TimeProviderTaskExtensions.cs#L13

All above made as internal and in a single file to simplify the sharing. Once .NET 8 is out we can nuke it and just use the standard package on lower frameworks.

.NET 8 target will still have no dependencies.

@martincostello
Copy link
Member Author

Yeah I was trying to do that but I got stuck once I tried to refactor the tests to account for not using those members added extra 😅

If next week you could take a look at that then I can rebase #1144 onto main and slim it down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 Issues related to the new version 8 of the Polly library.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants