mattmassicotte / ConcurrencyRecipes

Practical solutions to problems with Swift Concurrency
BSD 3-Clause "New" or "Revised" License
1.13k stars 34 forks source link

Update Protocols to include a new solution #6

Closed esnssr closed 8 months ago

esnssr commented 8 months ago

I updated the Protocols section to include a new solution for conformance to non-iscolated protocols that we don't have control over, e.g: system delegates. Would love to hear your thoughts and comments on this one as I implemented this in one of the projects I worked on! 😄

mattmassicotte commented 8 months ago

I think this is a great pattern to highlight, thank you! I brought up a few things that I think are worth addressing.

esnssr commented 8 months ago

I think this is a great pattern to highlight, thank you! I brought up a few things that I think are worth addressing.

Thanks for comments! I have addressed updated the PR with the requested changes :)

mattmassicotte commented 8 months ago

Ok, so here's my proposal. I'm going to merge this. You put in a bunch of effort and I really appreciate it! I also think the general pattern is definitely worth highlighting. However!

I think the wording in the contrast with solution #1 needs to be adjusted. And I think there's a more-straightforward solution that is less work, more explicit, and also better highlights the pros and cons.

protocol NotAsyncFriendly {
    func informational()

    func takesSendableArguments(_ value: Int)

    func expectsReturnValues() -> Int
}

actor MyActor {
    func doIsolatedThings(with value: Int) {

    }
}

extension MyActor: NotAsyncFriendly {
    nonisolated func informational() {
        Task {
            await self.doIsolatedThings(with: 0)
        }
    }

    nonisolated func takesSendableArguments(_ value: Int) {
        Task {
            // we can safely capture and/or make use of value here because it is Sendable
            await self.doIsolatedThings(with: value)
        }
    }

    nonisolated func expectsReturnValues() -> Int {
        // Because this function expects a synchronous return value, it is impossible to
        // access isolated state

        return 42
    }
}
mattmassicotte commented 8 months ago

And I've committed my substantial changes here 26c6abd2b1f4a681e486082399bbff09edd33162

I really hope you don't feel to bad about how much I've changed. I really do appreciate you highlighting this problem. It's a real one that definitely should be covered. But I wanted to make sure it fits with something that I can provide commentary around in my own voice. If you've found value in an intermediate wrapper above what I did, I'd love to hear more about it!

esnssr commented 8 months ago

And I've committed my substantial changes here 26c6abd

I really hope you don't feel to bad about how much I've changed. I really do appreciate you highlighting this problem. It's a real one that definitely should be covered. But I wanted to make sure it fits with something that I can provide commentary around in my own voice. If you've found value in an intermediate wrapper above what I did, I'd love to hear more about it!

On the contrary, I very much welcome and really really appreciate you taking the time to respond, interact with me, and improve on my solution. And again, I am still fresh to this newly world of Swift Concurrency, and am trying to learn from the community, and hopefully give back any knowledge I might have!

And as you said, this is a problem that should be highlighted, and I hope we get more clear solutions for all of these scenarios we currently run into when using Swift Concurrency.

Thanks again for your feedback! 🙏 And thanks for accepting the solution to be added to the repo 🥳

mattmassicotte commented 8 months ago

No need for thanks! And we're all leaning about concurrency, especially because it keeps changing 😅

So glad we got this in, and when you find more interesting problem, and I'm sure you will, let me know!