Swift Concurrency adoption guidelines for Swift Server Libraries #70
Conversation
ktoso
requested review from
ktoso,
0xTim,
adam-fowler,
glbrntt,
gwynne,
tomerd,
weissi,
yim-lee and
a team
|
Some follow ups we want to do:
|
|
Thanks David! |
There was a problem hiding this comment.
looks really good and I think this will be very helpful to library authors (including myself)! thank you both @fabianfett and @ktoso for your work on this.
the vast majority of my comments are just formatting / wording nits, adding links etc. to make things a little more readable.
| @@ -0,0 +1,179 @@ | |||
| # Swift Concurrency adoption guidelines for Swift Server Libraries | |||
There was a problem hiding this comment.
may be nice to add an "index" at the end before we merge so people can see the high level outline/jump to particular sections as this is getting rather lengthy
|
Thanks a lot for the edits @kmahar ! |
Co-authored-by: Kaitlin Mahar <kaitlinmahar@gmail.com>
|
|
||
| Most of these issues have been resolved on today’s `main` branch of the compiler, and are expected to land in the next Swift 5.5 releases. It may be worthwhile waiting for adoption until the next version(s) after 5.5.0. | ||
|
|
||
| For example, one of such capabilities is the ability for tuples of `Sendable` types to conform to `Sendable` as well. We recommend holding off adoption of `Sendable` until this patch lands in Swift 5.5 (which should be relatively soon). With this change, the difference between Swift 5.5 with `-warn-concurrency` enabled and Swift 6 mode should be very small, and manageable on a case by case basis. |
There was a problem hiding this comment.
It would be better to put an expected date in rather than relatively soon or this guide will not read well in a few months time.
|
|
||
| ### End-user code breakage | ||
|
|
||
| It is expected that Swift 6 will break some code. As mentioned Swift NIO 3 is also going to be released sometime around Swift 6 dropping. Keeping this in mind, it might be a good idea to align major version releases around the same time, along with updating version requirements to Swift 6 and NIO 3 in your libraries. |
There was a problem hiding this comment.
I wonder if "swift 6 dropping" is a term that everyone will recognise. eg, wondering what that would translate to if I ran this through google translate to a non-english language.
There was a problem hiding this comment.
That's a good point, a bit too "slang-y" I guess, thanks! I'll reword
Soto has an |
| In such situations, it may be helpful to utilize the following trick to be able to share the same `Container` declaration between both Swift versions of the library: | ||
|
|
||
| ```swift | ||
| #if compiler(>=5.5) |
There was a problem hiding this comment.
compiler is intended here and not swift?
There was a problem hiding this comment.
Can you remind me @Lukasa which one is right here?
There was a problem hiding this comment.
So in the end we need #if swift(>=5.5) && canImport(_Concurrency)
|
|
||
| To allow an easy transition to async code, SwiftNIO offers a number of helper methods on `EventLoopFuture` and `-Promise`. Those live in the `_NIOConcurrency` module and will move to `NIOCore` once Swift concurrency is released. | ||
|
|
||
| On every `EventLoopFuture` you can call `.get()` to transition the future into an `await`-able invocation. If you want to translate async/await calls to an `EventLoopFuture` we recommend the following pattern: |
There was a problem hiding this comment.
I know an example for this will be relatively trivial, but I think it would still be useful to include here. As a side benefit it'll highlight the fact that any such method will need to be throws as @adam-fowler points out above.
As part of that, it'll read a little clearer if the sentence about going the other direction (async -> EventLoopFuture) is broken off into its own line following the new example.
|
|
||
| While subject to change, it is likely that Swift NIO will cut a 3.0 release in the months after Swift 6.0, at which point in time Swift will have enabled “full” `Sendable` checking. | ||
|
|
||
| Do not expect NIO to suddenly become “more async”, NIO’s inherent design principles are about performing small tasks on the event loop and using Futures for any async operations. The design of NIO is not expected to change. It is crucial to its high performance networking design. Channel pipelines are not expected to become “async”. |
There was a problem hiding this comment.
The design of NIO is not expected to change. It is crucial to its high performance networking design.
This line seems to imply that async cannot be used without significant overhead, is this the case? If so, should we include guidance in this document about when not to completely adopt async, as NIO has opted not to? Or maybe a note saying NIO is a special case due to its low level nature, and that most/all other libraries should try to become more async?
The reason I mention this is that the majority of the document focuses on encouraging adoption of async, but at the very end here we have a note about how NIO will largely not be doing so, which may be a bit contradictory. A little bit further clarification could help with that though.
There was a problem hiding this comment.
NIO is not doing so because of the concerns around the “bottom” of the async pyramid. async/await is built on top of a co-operative thread pool within the Swift runtime. This thread pool must not be blocked by any operation, because doing so will starve the pool and prevent further progress of other async tasks.
This poses a problem for I/O. At some point, someone somewhere has to block a thread waiting for more I/O events, either in an I/O syscall or in something like epoll_wait. This is how NIO works: each of the event loop threads ultimately blocks on epoll_wait. We can’t do that inside the cooperative thread pool, as to do so would starve it for other async tasks, so we’d have to do so on a different thread.
This would mean we’d have a thread-hop between each I/O operation and dispatching it onto the async/await pool. This is not acceptable for high performance I/O: the context switch for each I/O op is too expensive. As a result, until we can safely block a thread for ourselves (with concurrent executors) we simply cannot use the async/await pattern in NIO code.
The rule of thumb is: if you need the absolute fastest computation on data from the network, you write ChannelHandlers and do your I/O on event loops. If you can tolerate slightly less performance there, likely because your business logic is more substantial, then async/await will repay you in ease of use much more than the (small) amount it’ll cost you in perf.
There was a problem hiding this comment.
That all makes sense, thanks for the detailed answer! I think it may be beneficial then to reword the existing paragraph to include some of this context and to be bit more explanatory rather than declarative (i.e. "NIO's API will largely remain the same because..." rather than "don't expect NIO's API to change..."). This will also help library authors decide when they might need to use NIO's facilities more directly rather than higher level async/await ones.
There was a problem hiding this comment.
Yeah, happy to take some of the wording into the doc 👍
| In order to have code using concurrency along with code not using concurrency, you may have to `#if` guard certain pieces of code. The correct way to do so is the following: | ||
|
|
||
| ```swift | ||
| #if compiler(>=5.5) && canImport(_Concurrency) |
There was a problem hiding this comment.
There are bunch of places in this guide which are missing && canImport(_Concurrency). There are also places where swift(>=5.5) instead of compiler(>=5.5).
There was a problem hiding this comment.
I think it's worth adding a note about @available requirements too.
|
|
||
| ## What you can do right now | ||
|
|
||
| ### `#if` guarding code using Concurrency |
There was a problem hiding this comment.
The subsections are roughly categorised as API, implementation and preparing for the future, so I think the "API design" and "SwiftNIO Helper Functions" sections should come before this one.
|
|
||
| If a function cannot fail but was using futures before, it should not include the `throws` keyword in its new incarnation. | ||
|
|
||
| Such adoption can begin immediately, and should not cause any issues to existing users of existing libraries. |
There was a problem hiding this comment.
I think there are broader changes that libraries can make as well, e.g. AsyncSequence based APIs rather than delegate based APIs, for example.
It may also be worth calling out that given we're on the road to Swift 6, libraries and the ecosystem as a whole are going to have to go through major version bumps so adding async-await APIs now can help lessen the pain of going through a major version change later because new APIs can be adopted incrementally.
|
|
||
| The NIO team will however use the chance to remove deprecated APIs and improve some APIs. The scope of changes should be comparable to the NIO1 → NIO2 version bump. If your SwiftNIO code compiles today without warnings, chances are high that it will continue to work without modifications in NIO3. | ||
|
|
||
| After the release of NIO3, NIO2 will see bug fixes only. |
There was a problem hiding this comment.
We'll fix security vulnerabilities too!
|
|
||
| ### Guidance for library users | ||
|
|
||
| As soon as Swift 6 comes out, we recommend using the latest Swift 6 toolchains, even if using the Swift 5.5.n language mode (which may yield only warnings rather than hard failures on failed Sendability checks). |
There was a problem hiding this comment.
Worth saying why we make this recommendation
There was a problem hiding this comment.
thanks
As soon as Swift 6 comes out, we recommend using the latest Swift 6 toolchains, even if using the Swift 5.5.n language mode (which may yield only warnings rather than hard failures on failed Sendability checks). This will result in better warnings and compiler hints, than just using a 5.5 toolchain.
| -> EventLoopFuture<Result> { | ||
| let promise = context.eventLoop.makePromise(of: Out.self) | ||
| promise.completeWithTask { | ||
| try await yourMethod(yourInputs) |
There was a problem hiding this comment.
Question about this; from what I can tell, this won't run yourMethod on an EventLoop; (assuming because I tried an example and MultiThreadedEventLoopGroup.currentEventLoop is nil).
Is there a way to have async tasks run on an event loop?
I ask since I thought that ideally during a request's lifecycle it won't be hopping off the EventLoop it's received on, even if running async tasks like a database lookup.
I might be confused on what's going on here too 🙂
There was a problem hiding this comment.
Is there a way to have async tasks run on an event loop?
Not today; that's what upcoming work on custom executors in Swift Concurrency would allow.
There was a problem hiding this comment.
The specific use case is that when running database queries, I've got a dict of connection pools, one for each EventLoop.
When a query is made, a pool is found using the current event loop's objectidentifier. But it seems like with async functions, they aren't running on an event loop and so if they need to run a database query it's not possible to look up a pool based on that.
Co-authored-by: George Barnett <gbarnett@apple.com>
Co-authored-by: Patrick Freed <patrick.freed@mongodb.com>
Co-authored-by: Yim Lee <yim_lee@apple.com>
|
Going through including feedback last time and will merge, thanks for all the comments! |
|
Ok i think i applied all changes. Please do feel free to send in PRs and keep this guide growing with more patterns and ideas! |
|
Meh can't edit this PR branch, so will merge and PR followups |
Adopt Swift Concurrency adoption guidelines for Swift Server Libraries (swift-server/guides#70). - Use #if compiler(>=5.5) && canImport(_Concurrency) to judge if Concurrency is available - Some clean up
This is a first draft for "Swift Concurrency adoption guidelines for Swift Server Libraries" that was written by @ktoso and me. Looking forward to your input.