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

[Docs] Improve timeout docs #1767

Merged
merged 3 commits into from
Nov 2, 2023
Merged

[Docs] Improve timeout docs #1767

merged 3 commits into from
Nov 2, 2023

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

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.

docs/strategies/timeout.md Outdated Show resolved Hide resolved
@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]

docs/strategies/timeout.md Outdated Show resolved Hide resolved
.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
@martintmk martintmk enabled auto-merge (squash) November 2, 2023 14:00
@martintmk martintmk disabled auto-merge November 2, 2023 14:00
@martintmk martintmk enabled auto-merge (squash) November 2, 2023 14:00
@martintmk martintmk merged commit c5d268d into main Nov 2, 2023
12 checks passed
@martintmk martintmk deleted the mtomka/docs-timeout-impr branch November 2, 2023 14:23
@peter-csala peter-csala mentioned this pull request Nov 2, 2023
4 tasks
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