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

DoUntilQuorum: provide causes when cancelling contexts, and don't log cancellation as an error #449

Merged
merged 5 commits into from
Dec 12, 2023

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Dec 11, 2023

What this PR does:

This PR improves the handling of context cancellations in DoUntilQuorum:

  • if a cause is provided when cancelling the parent context passed to DoUntilQuorum, that cause will be returned by DoUntilQuorum if it terminates due to cancellation (if no cause is available, context.Cause() will fall back to context.Canceled, preserving the existing behaviour)
  • DoUntilQuorum will provide a cause when cancelling contexts it controls
  • DoUntilQuorum will no longer mark the trace span as failed if the call fails due to cancellation

Note that this introduces a breaking change to the signature of DoUntilQuorumWithoutSuccessfulContextCancellation: the function provided must now accept a context.CancelCauseFunc instead of a context.CancelFunc.

Which issue(s) this PR fixes:

(none)

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@charleskorn charleskorn force-pushed the charleskorn/cancellation-improvements branch from 59cde43 to 27cf2ac Compare December 11, 2023 05:02
@aknuds1 aknuds1 added bug Something isn't working enhancement New feature or request labels Dec 11, 2023
Copy link
Collaborator

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM

@charleskorn charleskorn merged commit 6924834 into main Dec 12, 2023
3 checks passed
@charleskorn charleskorn deleted the charleskorn/cancellation-improvements branch December 12, 2023 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants