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

Should Clock.sleep(for:) be added? #59914

Closed
mbrandonw opened this issue Jul 6, 2022 · 12 comments · Fixed by #61222
Closed

Should Clock.sleep(for:) be added? #59914

mbrandonw opened this issue Jul 6, 2022 · 12 comments · Fixed by #61222
Labels
Concurrencу Area → standard library: The `Concurrency` module under the standard library umbrella feature A feature request or implementation standard library Area: Standard library umbrella swift evolution implemented Flag → feature: A feature that was approved through the Swift evolution process and implemented

Comments

@mbrandonw
Copy link
Contributor

The Clock proposal calls out 3 ways of sleeping, 2 on Task and 1 on Clock:

Task.sleep(until:)  // Instant-based
Task.sleep(for:)    // Duration-based
clock.sleep(until:) // Instant-based

However, there is no corresponding duration-based sleep for clocks:

clock.sleep(for:) // ❌ Duration-based

This means in order to perform duration-based sleeping, which is usually more ergonomic, you have to resort to the longer Task.sleep method:

try await Task.sleep(for: .seconds(1), clock: myClock)

// vs

try await myClock.sleep(for: .seconds(1))

Should this 4th variation of sleep be added to the standard library to be consistent?

@ktoso
Copy link
Contributor

ktoso commented Jul 7, 2022

It was in the accepted proposal, so I believe thats an omission -- right @phausler ?

@ktoso
Copy link
Contributor

ktoso commented Jul 7, 2022

Specifically the proposal listed:

extension Task {
  @available(*, deprecated, renamed: "Task.sleep(for:)")
  public static func sleep(_ duration: UInt64) async
  
  @available(*, deprecated, renamed: "Task.sleep(for:)")
  public static func sleep(nanoseconds duration: UInt64) async throws
  
  public static func sleep(for: Duration) async throws
  
  public static func sleep<C: Clock>(until deadline: C.Instant, tolerance: C.Instant.Duration? = nil, clock: C) async throws
}

https://github.com/apple/swift-evolution/blob/main/proposals/0329-clock-instant-duration.md

@ktoso ktoso added the concurrency Feature: umbrella label for concurrency language features label Jul 7, 2022
@phausler
Copy link
Contributor

phausler commented Jul 7, 2022

Correct, it was some of the very last iterations; the deprecations are obviously the next step but we can do those later to avoid churn.

@mbrandonw
Copy link
Contributor Author

Unless I'm missing something in the proposal, it does not seem that clock.sleep(for:) was ever proposed. There's clock.sleep(until:), and Task has both variations: Task.sleep(for:) and Task.sleep(until:).

Was it an oversight to not include clock.sleep(for:)? Does it require evolution approval to be added?

@ktoso ktoso removed the new feature label Jul 8, 2022
@mbrandonw
Copy link
Contributor Author

Any update on this? Can this be added before Swift 5.7 is finalized?

@mbrandonw
Copy link
Contributor Author

Just wanted to mention some concrete problems that arise from not having these functions.

Since all APIs dealing with clocks and sleeping use instants, and not durations, it is not possible to use the APIs with a Clock existential:

let clock: any Clock<Duration> = ContinuousClock()
try await clock.sleep(until: clock.now.advanced(by: .seconds(1)), tolerance: nil) // 🛑

This is because the Instant associated type gets erased but then the .sleep(until:) API forces us to construct instants.

If we had the sleep(for:) API we could make this work:

extension Clock {
  func sleep(for duration: Self.Duration) async throws {
    try await self.sleep(until: self.now.advanced(by: duration), tolerance: nil)
  }
}

let clock: any Clock<Duration> = ContinuousClock()
try await clock.sleep(for: .seconds(1)) // ✅

So, not having Clock.sleep(for:) and Task.sleep(for:clock:) makes it impossible to erase the type of a clock, which makes testing code that involves clocks very difficult.

Is there anything I can do to help move this forward? I can easily create a PR to add these functions to the standard library if it doesn't require evolution.

@mbrandonw
Copy link
Contributor Author

And one more thing to note. We definitely need the Clock.sleep(for:) method. The Task.sleep(for:clock:) static method alone is not sufficient due to a limitation of Swift 5.7 existentials which prevents you from opening the existential if a value of the associated type is used as an argument:

let clock: any Clock<Duration> = ContinuousClock()
try await Task.sleep(for: .seconds(1), clock: clock) // 🛑

@phausler
Copy link
Contributor

phausler commented Sep 19, 2022

Adding that function (to clock) would be an evolution type change.

@mbrandonw
Copy link
Contributor Author

Ok, posted a pitch: https://forums.swift.org/t/pitch-clock-sleep-for/60376

@benrimmington
Copy link
Contributor

The new and updated APIs from SE-0374 aren't available in Xcode 14.3 beta 2 (14E5207e).

I tried using an Xcode playground for iOS; and an iOS project with a 16.4 minimum deployment.

(Other APIs introduced in Swift 5.8 are available.)

@AnthonyLatsis AnthonyLatsis added feature A feature request or implementation standard library Area: Standard library umbrella swift evolution proposal needed Flag → feature: A feature that warrants a Swift evolution proposal swift evolution implemented Flag → feature: A feature that was approved through the Swift evolution process and implemented and removed swift evolution proposal needed Flag → feature: A feature that warrants a Swift evolution proposal labels Mar 10, 2023
@AnthonyLatsis AnthonyLatsis linked a pull request Mar 10, 2023 that will close this issue
@benrimmington
Copy link
Contributor

swiftlang/swift-evolution@419d000

-* Status: **Implemented (Swift 5.8)**
+* Status: **Implemented (Swift 5.9)**

@ktoso
Copy link
Contributor

ktoso commented Mar 31, 2023

Yeah seems this'll come in 5.9, I think we can close this issue

@ktoso ktoso closed this as completed Mar 31, 2023
@AnthonyLatsis AnthonyLatsis added Concurrencу Area → standard library: The `Concurrency` module under the standard library umbrella and removed concurrency Feature: umbrella label for concurrency language features labels Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concurrencу Area → standard library: The `Concurrency` module under the standard library umbrella feature A feature request or implementation standard library Area: Standard library umbrella swift evolution implemented Flag → feature: A feature that was approved through the Swift evolution process and implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants