Skip to content

Conversation

@Smaug123
Copy link
Contributor

The current documentation doesn't actually say what the semantics of the result are.

Copilot AI review requested due to automatic review settings September 12, 2025 12:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the documentation for the PeriodicTimer.WaitForNextTickAsync method by clarifying the boolean return value semantics and the different completion states of the returned ValueTask.

Key Changes

  • Enhanced the XML documentation to explicitly describe when the method returns true vs false
  • Added clarification about the cancellation behavior and task state

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 12, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 12, 2025
@jkotas jkotas added area-System.Threading documentation Documentation bug or enhancement, does not impact product or test code and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 12, 2025
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@steveisok steveisok requested a review from agocke October 13, 2025 20:15
Comment on lines 128 to 133
/// <returns>A <see cref="ValueTask" /> that will be completed due to the timer firing (in which case the result is <c>true</c>), <see cref="Dispose"/> being called to stop the timer (in which case the result is <c>false</c>), or cancellation being requested (in which case the task enters the Canceled state).</returns>
/// <remarks>
/// The <see cref="PeriodicTimer"/> behaves like an auto-reset event, in that multiple ticks are coalesced into a single tick if they occur between
/// calls to <see cref="WaitForNextTickAsync"/>. Similarly, a call to <see cref="Dispose"/> will void any tick not yet consumed. <see cref="WaitForNextTickAsync"/>
/// may only be used by one consumer at a time, and may be used concurrently with a single call to <see cref="Dispose"/>.
/// </remarks>
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to include more descriptive information of the returning ValueTask but including it in the returns section is not consistent with the repository. How about

Suggested change
/// <returns>A <see cref="ValueTask" /> that will be completed due to the timer firing, <see cref="Dispose"/> being called to stop the timer, or cancellation being requested.</returns>
/// <remarks>
/// The value of the Result property of the returned <see cref="ValueTask" /> is <see langword="true"/> when it's completed due to the timer firing and
/// <see langword="false"/> when the timer is disposed or stopped.
/// A cancelled task is returned if cancellation is requested.
/// The <see cref="PeriodicTimer"/> behaves like an auto-reset event, in that multiple ticks are coalesced into a single tick if they occur between
/// calls to <see cref="WaitForNextTickAsync"/>. Similarly, a call to <see cref="Dispose"/> will void any tick not yet consumed. <see cref="WaitForNextTickAsync"/>
/// may only be used by one consumer at a time, and may be used concurrently with a single call to <see cref="Dispose"/>.
/// </remarks>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I've adjusted the cancellation line because it's not precise.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Threading community-contribution Indicates that the PR has been added by a community member documentation Documentation bug or enhancement, does not impact product or test code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants