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

timeout should skip the timer if the delay is <= 0 #49

Closed
lilyball opened this issue May 20, 2020 · 1 comment
Closed

timeout should skip the timer if the delay is <= 0 #49

lilyball opened this issue May 20, 2020 · 1 comment
Labels
enhancement New feature or request
Milestone

Comments

@lilyball
Copy link
Owner

If the delay is zero or negative, instead of running a timer we should immediately check if the promise is resolved, and if not then time it out. This avoids an unnecessary timer and possibly operation queue work.

Implementation note: We should hop on the context, then run the timeout block there. Don't check it synchronously. The timeout block already does what we want and verifies that the promise hasn't resolved yet.

Design note: The above is different than checking result immediately and timing out if not. In particular, if we're using a context other than immediate, if the promise's upstream has resolved and the propagation is pending on that context, it may resolve the given promise prior to the timeout block firing. This matches current behavior. We should document that if the user wants to do the equivalent of if promise.result == nil { timeout() } then they can pass the .immediate context, or the nowOr(_) context (see #34).

@lilyball lilyball added the enhancement New feature or request label May 20, 2020
@lilyball
Copy link
Owner Author

It occurs to me that this will change the behavior of timeout(on: .immediate, delay: 0). With the old behavior that would timeout on the same queue as .auto, thus giving the promise a chance to resolve first, but with the new behavior it wouldn't. This is not documented behavior though.

Example:

DispatchQueue.main.async {
    let promise = Promise<Int,String>(on: .main, { (resolver) in
        resolver.fulfill(with: 42)
    }).timeout(on: .immediate, delay: 0)
    // old behavior will fulfill asynchronously on the main queue
    // new behavior will reject synchronously with a timeout error
}

@lilyball lilyball added this to the Next milestone May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant