retry.WithBackoff: QuickContextExit, Documentation, Tests #69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oxzi
added
enhancement
oxzi
added a commit
to Icinga/icingadb
that referenced
this pull request
Sep 5, 2024
A strange HA behavior was reported in [icingadb-787], resulting in both instances being active. The logs contained an entry of the previous active instance exiting the HA.realize() method successfully after 1m9s. This, however, should not be possible as the method's context is deadlined to a minute after the heartbeat was received. With introducing Settings.QuickContextExit for retry.WithBackoff in [icinga-go-library-69] and using it here, the function directly returns a context.DeadlineExceeded error the moment the context has expired. Doing so allows directly handing over, while the other instance can now take over due to the expired heartbeat in the database. As an related change, the HA.insertEnvironment() method was inlined into the retryable function to use the deadlined context. Otherwise, this might block afterwards, as it was used within HA.realize(), but without the passed context. In addition, the main loop select cases for hactx.Done() and ctx.Done() were unified, as hactx is a derived ctx. A closed ctx case may be lost as the hactx case could have been chosen. [icinga-go-library-69]: Icinga/icinga-go-library#69 [icingadb-787]: #787
oxzi
added a commit
to Icinga/icingadb
that referenced
this pull request
Sep 5, 2024
A strange HA behavior was reported in [icingadb-787], resulting in both instances being active. The logs contained an entry of the previous active instance exiting the HA.realize() method successfully after 1m9s. This, however, should not be possible as the method's context is deadlined to a minute after the heartbeat was received. With introducing Settings.QuickContextExit for retry.WithBackoff in [icinga-go-library-69] and using it here, the function directly returns a context.DeadlineExceeded error the moment the context has expired. Doing so allows directly handing over, while the other instance can now take over due to the expired heartbeat in the database. As a related change, the HA.insertEnvironment() method was inlined into the retryable function to use the deadlined context. Otherwise, this might block afterwards, as it was used within HA.realize(), but without the passed context. In addition, the main loop select cases for hactx.Done() and ctx.Done() were unified, as hactx is a derived ctx. A closed ctx case may be lost as the hactx case could have been chosen. [icinga-go-library-69]: Icinga/icinga-go-library#69 [icingadb-787]: #787
This was referenced Sep 5, 2024
julianbrost
reviewed
Sep 9, 2024
Comment on lines
+123
to
+141
| err = funcWrapper(func(ctx context.Context) error { | ||
| err := retryableFunc(ctx) | ||
| if err == nil { | ||
| if settings.OnSuccess != nil { | ||
| settings.OnSuccess(time.Since(start), attempt, prevErr) | ||
| } | ||
| } | ||
|
|
||
| return err | ||
| })(ctx) |
There was a problem hiding this comment.
Is the purpose of calling OnSuccess within the wrapped function so that this callback couldn't delay the return of this function either?
However, this seems to move calling the callbacks to another goroutine, which might be surprising and could introduce race conditions.
There was a problem hiding this comment.
Yes. Theoretically, both callback functions may block as well, while, unfortunately, they do not even receive a context to be bound to.
I am a bit unhappy with this architecture, but at least it does not break the API and needs a check for every retry.WithBackoff call. Another option would be to exclude the callbacks from the context-wrapper and stating this in the documentation. In most cases, the callbacks are only used for logging which should not block.
oxzi
added a commit
to Icinga/icingadb
that referenced
this pull request
Sep 10, 2024
A strange HA behavior was reported in [icingadb-787], resulting in both instances being active. The logs contained an entry of the previous active instance exiting the HA.realize() method successfully after 1m9s. This, however, should not be possible as the method's context is deadlined to a minute after the heartbeat was received. With introducing Settings.QuickContextExit for retry.WithBackoff in [icinga-go-library-69] and using it here, the function directly returns a context.DeadlineExceeded error the moment the context has expired. Doing so allows directly handing over, while the other instance can now take over due to the expired heartbeat in the database. As a related change, the HA.insertEnvironment() method was inlined into the retryable function to use the deadlined context. Otherwise, this might block afterwards, as it was used within HA.realize(), but without the passed context. In addition, the main loop select cases for hactx.Done() and ctx.Done() were unified, as hactx is a derived ctx. A closed ctx case may be lost as the hactx case could have been chosen. [icinga-go-library-69]: Icinga/icinga-go-library#69 [icingadb-787]: #787
The retry.WithBackoff function only loosely respected the provided context. In particular, the RetryableFunc could survive the context termination and block as long as it desires. For situations where timing is critical and a deadline bound context is used, this behavior might be undesirable. That's why the new Settings.QuickContextExit option has been introduced, which effectively wraps time-consuming, blocking functions with an additional context check and bails out directly when the context is done. This change was reflected in the documentation, which was generally extended, trying to explain more about the implementation details. While working on the function, some minor refactorings were done. Notable is the simplified check if the context has finished. Finally, tests were written for retry.WithBackoff to verify the expected behavior and ease future changes.
oxzi
force-pushed
the
retry-withbackoff-quickcontextexit
branch
from
ec93b83
to
45adf81
Compare
|
Since Icinga/icingadb#800 doesn't make use of this, is it still necessary? If we still intend to include it, it can be shipped in later releases, but it's not required for the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The retry.WithBackoff function only loosely respected the provided context. In particular, the RetryableFunc could survive the context termination and block as long as it desires.
For situations where timing is critical and a deadline bound context is used, this behavior might be undesirable. That's why the new Settings.QuickContextExit option has been introduced, which effectively wraps time-consuming, blocking functions with an additional context check and bails out directly when the context is done.
This change was reflected in the documentation, which was generally extended, trying to explain more about the implementation details.
While working on the function, some minor refactorings were done. Notable is the simplified check if the context has finished.
Finally, tests were written for retry.WithBackoff to verify the expected behavior and ease future changes.