belozierov / SwiftCoroutine

Swift coroutines for iOS, macOS and Linux.
https://belozierov.github.io/SwiftCoroutine
MIT License
836 stars 51 forks source link

`Coroutine.await` crashes for iOS #36

Closed funct7 closed 3 years ago

funct7 commented 3 years ago

Calling the below code crashes with Fatal error: Unexpectedly found nil while implicitly unwrapping an Optional value: file SwiftCoroutine/SharedCoroutine.swift, line 112.

The crashing version:

let scheduler = DispatchQueue.main
let repo = SomeRepository()

class SomeRepository {
    func getSomethingAsync() throws -> [String] {
        return try Coroutine.await { (continuation) in
            try! Coroutine.delay(.seconds(2))
            continuation(["foo", "bar", "baz",])
        }

    }
}

func foo() {
    scheduler.startCoroutine {
        do {
            let something = try repo.getSomethingAsync()
            print(something)
        } catch {
            #warning("TODO: handle error")
        }
    }
}

However, this is fine:

class SomeRepository {
    func getSomethingAsync() throws -> [String] {
        // getSomethingAsync is guaranteed to be called inside a coroutine
        try! Coroutine.delay(.seconds(2))
        return ["foo", "bar", "baz",]
    }
}

Not sure if this is expected behavior. If it is, I think it might be a good idea to mention dos and don'ts in the documentation.

belozierov commented 3 years ago

Hi @funct7, thank you for a very interesting case. The issue is because of nested Coroutine.await inside getSomethingAsync (inside Coroutine.delay there is other Coroutine.await). Coroutine.await is a very basic block that doesn't support nested behaviour. Your second getSomethingAsync is correct. Yes, you are right I should document it, throw an error or add such functionality. However, you can use nested CoroutineScheduler.await like this:

func getSomethingAsync() throws -> [String] {
   // Some `CoroutineScheduler.await`, for example` your `scheduler` or `DispatchQueue.global()`
   try scheduler.await {
      try Coroutine.delay(.seconds(2))
      return ["foo", "bar", "baz"]
   }
}

or with other coroutine like this (so we can use Coroutine.delay inside coroutine):

func getSomethingAsync() throws -> [String] {
   try Coroutine.await { continuation in
      scheduler.startCoroutine {
         try Coroutine.delay(.seconds(2))
         continuation(["foo", "bar", "baz"])
      }
   }
}

anyway inside Coroutine.await should be something async:

func getSomethingAsync() throws -> [String] {
   try Coroutine.await { continuation in
      // Something async
      DispatchQueue.global().async {
         continuation(["foo", "bar", "baz"])
      }
   }
}

Thanks again.

belozierov commented 3 years ago

@funct7 Also, I would recommend you to use CoFuture for getSomethingAsync(), it will make your code more readable, safer and easier to use:

/// Thanks to CoFuture you will know that it can be awaited
func getSomethingAsync() -> CoFuture<[String]> {
   CoFuture { promise in
      try? Coroutine.delay(.seconds(2))
      promise(.success(["foo", "bar", "baz"]))
   }
}

func foo() {
   scheduler.startCoroutine {
      do {
         let something = try repo.getSomethingAsync().await() // Await `CoFuture` result
         print(something)
      } catch {
         #warning("TODO: handle error")
      }
   }
}

Also, you can use CoFuture inside foo() for error handling:

func foo() {
   // Create CoFuture<Void> for which we can handle errors.
   scheduler.coroutineFuture {
      let something = try repo.getSomethingAsync().await() // Await `CoFuture` result
      print(something)
   }.whenFailure { error in
      #warning("TODO: handle error")
   }
}

There are a lot of ways how you can use it.

funct7 commented 3 years ago

Thanks for the comment. I was actually wondering what the best practice may be. Returning a CoFuture seemed a little like returning Deferred in Kotlin, while returning a result seemed like a suspend fun, at least at the Swift-language level. As long as all the results were to be awaited immediately after calling the function, I thought I might go with the suspend fun-looking version with some documentation. 😉

I agree that the caller await()ing makes it more explicit that the coroutine will be suspended.