contentful / contentful.swift

A delightful Swift interface to Contentful's content delivery API.
MIT License
203 stars 81 forks source link

Avoids calling completion twice on failure #363

Closed tristan-warner-smith closed 2 years ago

tristan-warner-smith commented 2 years ago

Swift's concurrency Task API will crash if completion handlers are called more than once in continuations. This is a critical issue affecting the Contentful SDK and this PR patches one case of it happening.

I encountered this because I've wrapped fetchArray(of in a Task and this triggers: Fatal error: SWIFT TASK CONTINUATION MISUSE: fetchArray(of:) tried to resume its continuation more than once, throwing Unknown error occured during decoding.! In my case I was mistakenly decoding a value using decode rather than decodeIfPresent.

Here's the client code to repro (if you decode erroneously as described above).

func fetchArray<ResourceType: EntryDecodable & FieldKeysQueryable>(of resourceType: ResourceType.Type) async throws -> [ResourceType] {

        try await withCheckedThrowingContinuation { continuation in
            fetchArray(of: resourceType) { result in
                switch result {
                case .success(let results):
                    continuation.resume(returning: results.items.map { $0 as ResourceType })
                case .failure(let error):
                    continuation.resume(throwing: error)
                }
            }
        }
    }

Client.swift:476 and 500 in Contentful 5.5.2.

tristan-warner-smith commented 2 years ago

The suggested approach of tracking it with a simple boolean isn't elegant but it's effective. As I understand it you always want to call churnLinks after an attempted decode even if it fails, I'm not sure if that's still a valid state but if it is then potentially wrapping the churnLinks code into a defer {} will allow you to remove the second decode attempt.

tristan-warner-smith commented 2 years ago

Updated as requested @mariuskatcontentful.

tristan-warner-smith commented 2 years ago

You could, but I read via other issues and PR attempts that the authors wanted to execute the linkResolver churnLinks codepath regardless of the parsers failing.

mariuskatcontentful commented 2 years ago

Updated as requested @mariuskatcontentful.

@tristan-reveri I do not think there is need to call churnLinks if the JSON parsin failed as links would have been already churned for each succeeded json before and if current JSON fails - there either would not be any links to churn or there would be the same "unchurnable" links that would not be able to resolve yet still. Any reasons why you would want that apart from my mentioned ones?

tristan-warner-smith commented 2 years ago

@mariuskatcontentful

I have no use case for maintaining that behaviour, I'd just seen that objection raised by Contentful devs against other similar issues and PRs.

I completely agree, I have no reason to maintain that behaviour, I thought I saw a very similar PR that addressed the multiple completion-call issue that way, I put this PR up in case the reason it hadn't been approved was due to the desire to maintain that churnLinks behaviour 👍

mariuskatcontentful commented 2 years ago

@tristan-reveri ok in that case i think if you could just adjust your PR to return just after the first error completion i will approve it immediately :)

tristan-warner-smith commented 2 years ago

Done @mariuskatcontentful 👍