-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
asyncBoot will no longer try booting server again if it is already booted #3195
Conversation
…erver is already booted
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3195 +/- ##
==========================================
+ Coverage 76.86% 77.46% +0.59%
==========================================
Files 211 210 -1
Lines 8119 8089 -30
==========================================
+ Hits 6241 6266 +25
+ Misses 1878 1823 -55
|
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.
Thanks! Could you add a test to ensure this works?
func testBootDoesNotTriggerHandlerMultipleTimes() throws {
let app = Application(.testing)
defer { app.shutdown() }
final class Handler: LifecycleHandler, Sendable {
let bootCount = NIOLockedValueBox(0)
func willBoot(_ application: Application) throws {
bootCount.withLockedValue { $0 += 1 }
}
}
let handler = Handler()
app.lifecycle.use(handler)
try app.boot()
try app.boot()
XCTAssertEqual(handler.bootCount.withLockedValue({ $0 }), 1)
}
func testAsyncBootDoesNotTriggerHandlerMultipleTimes() async throws {
let app = try await Application.make(.testing)
final class Handler: LifecycleHandler, Sendable {
let bootCount = NIOLockedValueBox(0)
func willBoot(_ application: Application) throws {
bootCount.withLockedValue { $0 += 1 }
}
}
let handler = Handler()
app.lifecycle.use(handler)
try await app.asyncBoot()
try await app.asyncBoot()
XCTAssertEqual(handler.bootCount.withLockedValue({ $0 }), 1)
try await app.asyncShutdown()
}
In ApplicationTests should work
…lifecycke handlers only once
Done. Please have a look 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.
Thanks!
These changes are now available in 4.100.2
The synchronous
boot
function skips running the lifecycle handlers if the server is already booted. However, the async version ignored this check. I have added a small fix to add this check again.