Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Structured concurrency for server applications #447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Structured concurrency for server applications #447
Changes from 1 commit
acd2fc7
eaf02d8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we say:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can be a bit more general here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the more general approach leads to clear communication of our intend here:
For client requests, we have clear rules:
Everything that we not consider part of a client request we dispatch into the background task via AsyncSequence, whereas the original client request remains within whatever task the user scheduled the work in. for the background work we need a task root. This is the reason why we need a
run()
for clients.For server tasks:
We need a task root to schedule the request-response handling in. This is a behavior that is contrary to clients.
--
This distinction is extremely important to ensure that cancellation works correctly in both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I have rewritten this part and actually added some code examples for a simplified
HTTPClient
andHTTPServer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it sound like a garbage collector, but ARC is deterministic, so maybe tweak the wording here to instead focus on the fact that it can be difficult to enforce cleanup at a specific time when a reference is shared between tasks. That said, this might not be important, for example if you have 5 identical worker tasks sharing a resource, and they're all shutting down, you might not care which one is the last one to shut down (and thus allow the resource to be deinited), so maybe add an example of where this is important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if in-scope for this guide, or if it should be a separate one, but once I have these structured tasks nicely set up, how do I communicate between them? More guidance on that topic would be great, especially how to wire up the async sequences (I presume) at task creation time, some patterns around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even more importantly, a non-copyable type can represent a resource like this, where you do have full control of when it's cleaned up, making the scope-based API unnecessary. If you're linking to non-escaping, might be worth linking to non-copyable as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with
~Copyable
and~Escapable
types we still can't express deinit based resource clean up in all cases. Closing FDs or deleting VMs is an asynchronous action anddeinit
s cannot be async at this time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, but "in all cases" is a very high bar. Non-copyable types help substantially over the status quo, by allowing a single owner known at compile time, who's responsible for managing the resource. When and how that owner chooses to free resources is orthogonal, but the important part is that you can achieve deterministic resource management without a
with
style API, which was my original point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still cannot. Anything that requires an asynchronous deinit cannot achieve deterministic resource management. Deterministic means at any point in your program you can tell when the resource is freed which works for simple things with
~Copyable
but won't work for stuff like file descriptors, sockets or virtual machines.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow. If I have a non-copyable type with a
func close() async
on it which must be called before deinit (otherwise a precondition fails), by looking at the code you know exactly when it's being freed, and when the freeing finished.I'm not talking about deinit doing the work, I'm saying that the lack of non-copyable types meant that the only way to enforce a single known entity to free the resource was a
with
API. With non-copyable types, there is always exactly one owner of the value, who's responsible for freeing it (in any way that makes sense for the type, for sync types, it can be using deinit, for async types it can be using an explicit close, whatever).I don't see any non-determinism here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, async deinit would make this even more flexible, but it's not a blocker for using non-copyable types correctly even without
with
style APIs. (You still can, of course, it's just that now there's a second way.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before we go into NIO land here, we should probably create something like how Server applications in structured concurrency:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then you should probably explain how handConnection should consume the connections incoming messages as an AsyncSequence. Again. Local reasoning! All code is right there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you have established this pattern, you can explain that a NIOAsyncChannel works exactly like your connection here. Then link to the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added an example above for the server which I am going to pick up here again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually just moved the whole section around task executors out. Let's keep it focused on structured concurrency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIO
is a module, I think we should be calling it SwiftNIO hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I would mention this in this document even.