Skip to content
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

Introduce a new nonblocking run() method on Application #2938

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions Sources/Vapor/Application.swift
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,12 @@ public final class Application {
DotEnvFile.load(for: environment, on: .shared(self.eventLoopGroup), fileio: self.fileio, logger: self.logger)
}

/// Starts the Application using the `start()` method, then waits for any running tasks to complete
#if swift(>=5.7)
/// Starts the Application using the `start()` method, then blocks the thread while any tasks are running
/// If your application is started without arguments, the default argument is used.
///
/// Under normal circumstances, `run()` begin start the shutdown, then wait for the web server to (manually) shut down before returning.
/// Under normal circumstances, `run()` starts the webserver, then wait for the web server to (manually) shut down before returning.
@available(*, noasync)
public func run() throws {
do {
try self.start()
Expand All @@ -104,6 +106,35 @@ public final class Application {
throw error
}
}
#else
/// Starts the Application using the `start()` method, then blocks the thread while any tasks are running
/// 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.g
public func run() throws {
do {
try self.start()
try self.running?.onStop.wait()
} catch {
self.logger.report(error: error)
throw error
}
}
#endif

/// Starts the Application using the `start()` method, then awaits the end of any tasks that are running
/// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been my repeated experience that this must be marked @MainActor in order to work reliably.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that so? I've not needed that, shall I change it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing in the server world should require @MainActor. Maybe @gwynne hit some Vapor thread-safety issues that were accidentally worked around by @MainActor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so already @weissi

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gwynne when you have a repro, try swift build -c release --sanitize=thread and then run that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gwynne is this PR a go then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

do {
try self.start()
try await self.running?.onStop.get()
} catch {
self.logger.report(error: error)
throw error
}
}

/// When called, this will execute the startup command provided through an argument. If no startup command is provided, the default is used.
/// Under normal circumstances, this will start running Vapor's webserver.
Expand Down