Introduce a new nonblocking run() method on Application
#2938
Conversation
| /// If your application is started without arguments, the default argument is used. | ||
| /// | ||
| /// Under normal circumstances, `run()` starts the webserver, then wait for the web server to (manually) shut down before returning. | ||
| public func run() async throws { |
There was a problem hiding this comment.
It has been my repeated experience that this must be marked @MainActor in order to work reliably.
There was a problem hiding this comment.
Is that so? I've not needed that, shall I change it?
There was a problem hiding this comment.
Nothing in the server world should require @MainActor. Maybe @gwynne hit some Vapor thread-safety issues that were accidentally worked around by @MainActor?
There was a problem hiding this comment.
I thought so already @weissi
There was a problem hiding this comment.
@weissi Yeah, that was my assumption, but I lacked the necessary tooling (and spare moments) at the time to dig into it further. It's definitely not a real solution.
There was a problem hiding this comment.
@gwynne when you have a repro, try swift build -c release --sanitize=thread and then run that
There was a problem hiding this comment.
@weissi Will do, but it occurred to me a few minutes ago that what probably happened is I ended up attempting to perform the stop from the same event loop on which the future would complete (before bridging back over to Concurrency), which was already invalid before async came along — after all, the old version is a .wait(), it's a guaranteed deadlock!
There was a problem hiding this comment.
@gwynne is this PR a go then?
There was a problem hiding this comment.
There's still nothing in place that ensures that, essentially, awaiting the onStop promise happens on the same thread that called start() - I specifically mean "thread" here, not "workqueue worker", which for concurrency at the moment will be a concurrent DispatchQueue and therefore might be any old OS-level thread. As best I can work out, the actual issue is that something in the lifecycle (during boot, or command running, server setup and teardown, whatever) makes an unspoken and probably completely accidental assumption of always being on the main thread/queue, so that awaiting the stop promise can effectively make it complete too soon, or never. Again, needs more investigation.
There was a problem hiding this comment.
@gwynne I don't see how that potential scenario would differ from the existing implementation - which uses .wait(). There's no asynchronous/offloaded work happening inside app.start() that would cause the console command (defaults to serve) to delay creating a server socket.
While I'd concur if it's sub-optimal to - for example - create a TCP server socket descriptor in a non-blocking fashion, that would be out of scope for this PR and a breaking change. So what do you suggest we do?
|
Superseded by #3114 |
Fixes #2937
Be careful releasing this.
run()