Skip to content

Conversation

@martintmk
Copy link
Contributor

@martintmk martintmk commented Nov 2, 2023

Details on the issue fix or feature implementation

Added more details to timeout strategy and introduced anti-patterns section.

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

@codecov
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (98f29c8) 84.53% compared to head (83c51bc) 84.53%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1767   +/-   ##
=======================================
  Coverage   84.53%   84.53%           
=======================================
  Files         307      307           
  Lines        6777     6777           
  Branches     1043     1043           
=======================================
  Hits         5729     5729           
  Misses        839      839           
  Partials      209      209           
Flag Coverage Δ
linux ?
macos ?
windows 84.53% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...c/Polly.Core/Retry/RetryStrategyOptions.TResult.cs 100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@martintmk martintmk enabled auto-merge (squash) November 2, 2023 12:57

## Anti-patterns

### Ignoring Cancellation Token
Copy link
Contributor

Choose a reason for hiding this comment

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

On other pages the anti-patterns are numbered. I think it might make sense to add numbering here as well

Suggested change
### Ignoring Cancellation Token
### 1 - Ignoring Cancellation Token

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the number add any value though?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you have multiple paragraphs and one might refer to a previous one then its number could enough.

But I also did not follow this :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me delete all the numbers in my next PR where I change the [!IMPORTANT] blocks to [!INFO]

.Build();

await pipeline.ExecuteAsync(
async innerToken => await Task.Delay(3000, outerToken), // The delay call should use innerToken
Copy link
Contributor

Choose a reason for hiding this comment

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

In every other places we use TimeSpan.FromXYZ in the examples. I would suggest to use here as well.

Suggested change
async innerToken => await Task.Delay(3000, outerToken), // The delay call should use innerToken
async innerToken => await Task.Delay(TimeSpan.FromSeconds(3), outerToken), // The delay call should use innerToken


**Reasoning**:

The provided callback respects the `innerToken` provided by the pipeline, and as a result, the callback is correctly cancelled by the timeout strategy after 1 second.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The provided callback respects the `innerToken` provided by the pipeline, and as a result, the callback is correctly cancelled by the timeout strategy after 1 second.
The provided callback respects the `innerToken` provided by the pipeline, and as a result, the callback is correctly cancelled by the timeout strategy after 1 second plus `TimeoutRejectedException` is thrown.

@martintmk martintmk disabled auto-merge November 2, 2023 13:30
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.

3 participants