[swift5] Cancellable requests by davidhorvath · Pull Request #10855 · OpenAPITools/openapi-generator

davidhorvath · 2021-11-14T14:26:15Z

resolve #8658

Previously the generated clients didn't support cancellation of requests

I've added request cancellation for:

Both URLSession & Alamofire library
Combine configuration
RxSwift configuration
Result configuration
AsyncAwait configuration

For these clients I have updated the RequestBuilder's execute function so it returns an optional URLSessionDataTask.
I've also added @discardableResult to keep backward compatibility.

Default

I've added to return the URLSessionDataTask so the user will be able to control the request's lifecycle.

    @discardableResult
    open class func addPet(body: Pet, apiResponseQueue: DispatchQueue = PetstoreClientAPI.apiResponseQueue, completion: @escaping ((_ data: Void?, _ error: Error?) -> Void)) -> URLSessionDataTask? {
        return addPetWithRequestBuilder(body: body).execute(apiResponseQueue) { result in
            switch result {
            case .success:
                completion((), nil)
            case let .failure(error):
                completion(nil, error)
            }
        }
    }

Result

I've added to return the URLSessionDataTask so the user will be able to control the request's lifecycle.

    @discardableResult
    open class func addPet(body: Pet, apiResponseQueue: DispatchQueue = PetstoreClientAPI.apiResponseQueue, completion: @escaping ((_ result: Swift.Result<Void, ErrorResponse>) -> Void)) -> URLSessionTask? {
        return addPetWithRequestBuilder(body: body).execute(apiResponseQueue) { result in
            switch result {
            case .success:
                completion(.success(()))
            case let .failure(error):
                completion(.failure(error))
            }
        }
    }

Combine

I've implemented handeEvents(receiveCompletion: where I cancel the returned URLSessionDataTask when the stream is cancelled

open class func addPet(body: Pet, apiResponseQueue: DispatchQueue = PetstoreClientAPI.apiResponseQueue) -> AnyPublisher<Void, Error> {
        var dataTask: URLSessionDataTask?
        return Future<Void, Error> { promise in
            dataTask = addPetWithRequestBuilder(body: body).execute(apiResponseQueue) { result in
                switch result {
                case .success:
                    promise(.success(()))
                case let .failure(error):
                    promise(.failure(error))
                }
            }
        }
        .handleEvents(receiveCancel: {
            dataTask?.cancel()
        })
        .eraseToAnyPublisher()
    }

RxSwift

I've added to cancel the returned URLSessionDataTask in Disposables.create when the stream is cancelled

    open class func addPet(body: Pet, apiResponseQueue: DispatchQueue = PetstoreClientAPI.apiResponseQueue) -> Observable<Void> {
        return Observable.create { observer -> Disposable in
            let dataTask = addPetWithRequestBuilder(body: body).execute(apiResponseQueue) { result in
                switch result {
                case .success:
                    observer.onNext(())
                case let .failure(error):
                    observer.onError(error)
                }
                observer.onCompleted()
            }
            
            return Disposables.create {
                dataTask?.cancel()
            }
        }
    }

Async Await

I've added to cancel the returned URLSessionDataTask in withTaskCancellationHandler.onCancel:

    open class func addPet(body: Pet, apiResponseQueue: DispatchQueue = PetstoreClientAPI.apiResponseQueue) async throws {
        var task: URLSessionTask?
        return try await withTaskCancellationHandler {
            try Task.checkCancellation()
            return try await withCheckedThrowingContinuation { continuation in
                guard !Task.isCancelled else {
                  continuation.resume(throwing: CancellationError())
                  return
                }

                task = addPetWithRequestBuilder(body: body).execute(apiResponseQueue) { result in
                    switch result {
                    case .success:
                        continuation.resume(returning: ())
                    case let .failure(error):
                        continuation.resume(throwing: error)
                    }
                }
            }
        } onCancel: { [task] in
            task?.cancel()
        }
    }

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request. @4brunu

davidhorvath · 2021-11-14T14:31:29Z

@4brunu I have one question about this.
Don't we want to wrap this Future publisher into a Deferred publisher?
So it would work like Rx's Single by having it defer its execution until it receives a subscriber.

I don't have much experience with RxSwift or Combine.
Is the current RxSwift integration lazy (defer its execution until it receives a subscriber)?

@4brunu Yes. RxSwift Observable's only call when you subscribe to the stream.
In Combine publishers are working the same way except Future. We can only achieve the same if we would wrap Future in Deferred.

So currently if we just write this:

let newPet = Pet(id:<id>, category: <category>, name: <name>, photoUrls: []) let addPetPublisher = PetAPI.addPet(body: newPet)

The request will be called without subscribing to the stream with .sink

This is the way it is, because when I introduced support for Combine I didn't have much experience with it, and Future looked like a good fit.
I think we can also make Combine lazy, because that would make the API more flexible, by only starting the request when the user want's.
What do you think?

Yes, I think we should do that 👍

Can this behaviour change create any bugs for the users that already implement Combine in their projects?
For example a network request that never happens because of this change?

Unfortunately, yes 😕

If somebody used it without subscribing to the stream, it won't be called

Hum, let's keep that for a different PR then, and keep this one with just the cancellation, to avoid any friction to merge it.

Oh yeah for sure

resolve OpenAPITools#8658

4brunu · 2021-11-14T21:06:52Z

Hey @davidhorvath, this is another great PR, thanks 👍
Here are some thoughts.

Instead of returning an URLSessionDataTask I think we should return a URLSessionTask because it's less specialised.

We should also support cancellation in Alamofire, by replace the following code

if let encoding = encoding {
    let request = makeRequest(manager: manager, method: xMethod, encoding: encoding, headers: headers)
    if let onProgressReady = self.onProgressReady {
        onProgressReady(request.uploadProgress)
    }
    processRequest(request: request, managerId, apiResponseQueue, completion)
    return request.task
}

return nil

In the following class

openapi-generator/modules/openapi-generator/src/main/resources/swift5/libraries/alamofire/AlamofireImplementations.mustache

Lines 138 to 144 in f596b32

    
                   if let encoding = encoding { 
        
                       let request = makeRequest(manager: manager, method: xMethod, encoding: encoding, headers: headers) 
        
                       if let onProgressReady = self.onProgressReady { 
        
                           onProgressReady(request.uploadProgress) 
        
                       } 
        
                       processRequest(request: request, managerId, apiResponseQueue, completion) 
        
                   }

Those two changes should a lot of conditions in the templates.

We should also support request cancelation in result by returning the URLSessionTask and async/await by using the Task but we need to explore this a bit more, because I don't have much experience with async/await.
https://www.hackingwithswift.com/quick-start/concurrency/how-to-cancel-a-task
https://desiatov.com/swift-structured-concurrency-introduction/
https://betterprogramming.pub/swiftui-concurrency-understand-cooperative-task-cancellation-b71cc69483e0

4brunu · 2021-11-14T22:16:17Z

Looks like the way to cancel an async/await function is https://github.com/apple/swift-evolution/blob/main/proposals/0304-structured-concurrency.md#cancellation-handlers

Here is the example in the like above.

func download(url: URL) async throws -> Data? {
  var urlSessionTask: URLSessionTask?

  return try withTaskCancellationHandler {
    return try await withUnsafeThrowingContinuation { continuation in
      urlSessionTask = URLSession.shared.dataTask(with: url) { data, _, error in
        if let error = error {
          // Ideally translate NSURLErrorCancelled to CancellationError here
          continuation.resume(throwing: error)
        } else {
          continuation.resume(returning: data)
        }
      }
      urlSessionTask?.resume()
    }
  } onCancel: {
    urlSessionTask?.cancel() // runs immediately when cancelled
  }
}

davidhorvath · 2021-11-16T19:12:03Z

Looks like the way to cancel an async/await function is https://github.com/apple/swift-evolution/blob/main/proposals/0304-structured-concurrency.md#cancellation-handlers

Here is the example in the like above.

func download(url: URL) async throws -> Data? {
  var urlSessionTask: URLSessionTask?

  return try withTaskCancellationHandler {
    return try await withUnsafeThrowingContinuation { continuation in
      urlSessionTask = URLSession.shared.dataTask(with: url) { data, _, error in
        if let error = error {
          // Ideally translate NSURLErrorCancelled to CancellationError here
          continuation.resume(throwing: error)
        } else {
          continuation.resume(returning: data)
        }
      }
      urlSessionTask?.resume()
    }
  } onCancel: {
    urlSessionTask?.cancel() // runs immediately when cancelled
  }
}

@4brunu Unfortunately this is not working the way it suppose to 😕
I get a Reference to captured var 'urlSessionTask' in concurrently-executing code compile error.
https://forums.swift.org/t/how-to-cancel-a-publisher-when-using-withtaskcancellationhandler/49688

davidhorvath · 2021-11-16T19:34:35Z

@4brunu
I've added a new commit where I

Changed URLSessionDataTasks to URLSessionTasks
Added support for request cancellation for alamofire library based on your comments
Return the task in Result configuration

Still missing:

Async/Await (I commented above that unfortunately that solution isn't working, I need to investigate further)

4brunu · 2021-11-16T21:06:45Z

@davidhorvath I think I got async/await working with cancellation

    @available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *)
    open class func fakeOuterBooleanSerialize1(body: Bool? = nil, apiResponseQueue: DispatchQueue = PetstoreClientAPI.apiResponseQueue) async throws -> Bool {
        var task: URLSessionTask?
        return try await withTaskCancellationHandler {
            try await withCheckedThrowingContinuation { continuation in
                task = fakeOuterBooleanSerializeWithRequestBuilder(body: body).execute(apiResponseQueue) { result in
                    switch result {
                    case let .success(response):
                        continuation.resume(returning: response.body!)
                    case let .failure(error):
                        continuation.resume(throwing: error)
                    }
                }
            }
        } onCancel: { [task] in
            task?.cancel()
        }
    }

Just for reference here are the links where I found the solution.
https://github.com/futuredapp/FTAPIKit/blob/6f6c8128ef62e160c070afeeb7d0c025ca336a2e/Sources/FTAPIKit/URLServer%2BAsync.swift
https://github.com/alexito4/RuneterraWallpapersDownloader/blob/332d49f0c92d4fecfd8616a7d788a3dcde18c45e/Sources/RuneterraWallpapersDownloader/URLSessionAsync.swift
https://www.pointfree.co/episodes/ep154-async-refreshable-composable-architecture (in the first exercise that use withTaskCancellationHandler)

4brunu · 2021-11-17T19:47:36Z

@davidhorvath I don't want to be annoying, but I got one last improvement that it would be nice, but if you prefer I can do it in a separated PR.
In the async/await we could check for cancellation, like so:

    @available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *)
    open class func call123testSpecialTags(body: Client, apiResponseQueue: DispatchQueue = PetstoreClientAPI.apiResponseQueue) async throws -> Client {
        var task: URLSessionTask?
        return try await withTaskCancellationHandler {
            try Task.checkCancellation() // HERE
            return try await withCheckedThrowingContinuation { continuation in // also add return in the beginning of the line
                guard !Task.isCancelled else { // and HERE
                  continuation.resume(throwing: CancellationError())
                  return
                }
                task = call123testSpecialTagsWithRequestBuilder(body: body).execute(apiResponseQueue) { result in
                    switch result {
                    case let .success(response):
                        continuation.resume(returning: response.body!)
                    case let .failure(error):
                        continuation.resume(throwing: error)
                    }
                }
            }
        } onCancel: { [task] in
            task?.cancel()
        }
    }

What do you think?
I got this idea from here
https://www.pointfree.co/episodes/ep154-async-refreshable-composable-architecture (in the first exercise that use withTaskCancellationHandler)

davidhorvath · 2021-11-17T20:17:07Z

@davidhorvath I don't want to be annoying, but I got one last improvement that it would be nice, but if you prefer I can do it in a separated PR. In the async/await we could check for cancellation, like so:

    @available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *)
    open class func call123testSpecialTags(body: Client, apiResponseQueue: DispatchQueue = PetstoreClientAPI.apiResponseQueue) async throws -> Client {
        var task: URLSessionTask?
        return try await withTaskCancellationHandler {
            try Task.checkCancellation() // HERE
            return try await withCheckedThrowingContinuation { continuation in // also add return in the beginning of the line
                guard !Task.isCancelled else { // and HERE
                  continuation.resume(throwing: CancellationError())
                  return
                }
                task = call123testSpecialTagsWithRequestBuilder(body: body).execute(apiResponseQueue) { result in
                    switch result {
                    case let .success(response):
                        continuation.resume(returning: response.body!)
                    case let .failure(error):
                        continuation.resume(throwing: error)
                    }
                }
            }
        } onCancel: { [task] in
            task?.cancel()
        }
    }

What do you think? I got this idea from here https://www.pointfree.co/episodes/ep154-async-refreshable-composable-architecture (in the first exercise that use withTaskCancellationHandler)

@4brunu Sure I can add that as well

4brunu

Looks good to me 👍
@davidhorvath Thank you very much for the two awesome PRs, that were very impactful in the Swift Generator, and for your quality PRs.
Thanks you also for your patience and for doing the required changes to merge the PRs.
Are you planning to do open more PRs? 😃
Feel free to contribute any time you want 👍

davidhorvath commented Nov 14, 2021

View reviewed changes

davidhorvath added 2 commits November 14, 2021 16:05

Implement cancellation of URLSession requests for Combine & RxSwift

31f75b4

resolve OpenAPITools#8658

Add generated sample code

eb542d6

davidhorvath force-pushed the swift5-cancel-requests branch from a28ddcc to eb542d6 Compare November 14, 2021 15:07

4brunu reviewed Nov 14, 2021

View reviewed changes

Comment thread modules/openapi-generator/src/main/resources/swift5/APIs.mustache Outdated