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

Improve Gate::close to support repeated calls #662

Merged
merged 2 commits into from
May 13, 2024

Conversation

vlovich
Copy link
Contributor

@vlovich vlovich commented May 11, 2024

What does this PR do?

If Gate::close has a timeout, closing the gate is left in a dangling state with no way to safely resume waiting. This change makes it safe in that calling close on a closing Gate still waits for any outstanding tasks. This way you can repeatedly try calling gate close with a timeout set while safely waiting for all tasks to complete.

This is technically an observable change in that a previous call to close when still in "closing" would have returned an error but now waits for the currently running tasks instead. I can't imagine anyone actually relies too much on this.

Motivation

I have a gate that protects whether or not I've started doing I/O. My close routine awaits the gate closure. However, there's a risk that I have a timeout hit that cancels the waiting for close. If I try a subsequent close, it'll have no choice but to skip calling close even though there may be tasks active, resulting in entering a piece of code unexpectedly that the close was supposed to protect.

With this change, I have a way to now force that the gate is always in a close state regardless of timeouts by invoking close if !is_closed.

Related issues

Additional Notes

Checklist

[X] I have added unit tests to the code I am submitting
[X] My unit tests cover both failure and success scenarios
[] If applicable, I have discussed my architecture

If Gate::close has a timeout, closing the gate is left in a dangling
state with no way to safely resume waiting. This change makes it safe in
that calling close on a closing Gate still waits for any outstanding
tasks. This way you can repeatedly try calling gate close with a timeout
set while safely waiting for all tasks to complete.

This is technically an observable change in that a previous call to close
when still in "closing" would have returned an error but now waits for the
currently running tasks instead. I can't imagine anyone actually relies
too much on this.
@vlovich
Copy link
Contributor Author

vlovich commented May 11, 2024

Updated the PR to roll in another enhancement I've wanted, which is for Gate close to be synchronous & return an asynchronous for the waiter. That way Gate::close is guaranteed to apply before the future future is awaited. It's useful for this closure pattern where:

let was_open = gate.is_open();
let closure = gate.close();
if was_open { 
    // This is the only dirty state possible because any straggling I/O are guaranteed to see the gate as closing
    // and will return an error.
    complete_outstanding_work().await?;
}
closure.await? // We're now guaranteed that there's no outstanding writes.

This might be a breaking change if anyone relied on the closure not applying until the future awaited so this would warrant a bump to 0.10 for the next release & release notes.

It's useful if the pattern you want to do is to initiate a close and
then wait for it completing later. Otherwise you sometimes have to jump
through hoops.
/// NOTE: After this function returns, [is_open](Self::is_open) returns false and any subsequent attempts to acquire
/// a pass will fail, even if you drop the future. The future will return an error if and only if the gate is
/// already fully closed
pub fn close(&self) -> impl Future<Output = Result<(), GlommioError<()>>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be Future<Output = Result> or Result<Output = Future>? The latter is a cleaner API but the approach taken here means the users of the library don't need to make any code changes when upgrading. On the other hand, they may be silently impacted by a slight behavioral change in the mechanics of the close which may or may not be important.

I went with the "no code change required to upgrade" approach since I figure the behavioral change is unlikely to be noticed but let me know which approach is preferrable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what you did is better.

I don't think the latter is a cleaner API, anyway, but since you're not pushing for it, what I think is immaterial =)

@glommer glommer merged commit c373d35 into DataDog:master May 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants