Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix test hangs by replacing synchronous syncShutdownGracefully() calls with new async shutdown helper functions. The change affects how NIO EventLoopGroups and ThreadPools are cleaned up in test teardown.
- Introduces two new helper functions:
shutdownEventLoopGroupAsync()andshutdownThreadPoolAsync() - Replaces all
try? elg.syncShutdownGracefully()andtry? pool.syncShutdownGracefully()calls with the new async helpers - Refactors
MemoryAdapterAsyncTeststo use awithAdapterhelper that manages EventLoopGroup lifecycle
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CasbinTests/TestSupport.swift | Adds Dispatch import and two new async shutdown helper functions that use fire-and-forget pattern with background queue |
| Tests/CasbinTests/RbacApiTests.swift | Updates defer block in withEnforcer to use new async shutdown helpers instead of sync shutdown |
| Tests/CasbinTests/MemoryAdapterAsyncTests.swift | Refactors from makeAdapter() to withAdapter() helper pattern and updates to use async shutdown in defer block |
| Tests/CasbinTests/EnforcerTests.swift | Updates defer block in withELGAndPool to use new async shutdown helpers instead of sync shutdown |
| Tests/CasbinTests/ConfigTests.swift | Updates defer blocks in both test methods to use new async shutdown helpers instead of sync shutdown |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Tests/CasbinTests/TestSupport.swift
Outdated
| func tryBool(_ body: () throws -> Bool) -> Bool { (try? body()) ?? false } | ||
|
|
||
| // Non-blocking shutdown helpers so cooperative test executors don't hang. | ||
| func shutdownEventLoopGroupAsync(_ group: EventLoopGroup) { |
There was a problem hiding this comment.
The function name shutdownEventLoopGroupAsync is misleading. In Swift async/await conventions, "async" typically refers to async functions that use structured concurrency. This function is actually performing a "fire-and-forget" operation. Consider naming it shutdownEventLoopGroupInBackground or beginShutdownEventLoopGroup to better reflect its behavior.
| func shutdownEventLoopGroupAsync(_ group: EventLoopGroup) { | |
| func shutdownEventLoopGroupInBackground(_ group: EventLoopGroup) { |
|
@zamderax modify based on review: #55 (review) |
- Add dedicated async and background shutdown helper functions - Replace defer blocks with InBackground variants for fire-and-forget cleanup - Use explicit await for async shutdown in MemoryAdapterAsyncTests - Prevents test hanging issues by properly managing EventLoopGroup and NIOThreadPool lifecycle
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func shutdownThreadPoolInBackground(_ pool: NIOThreadPool) { | ||
| pool.shutdownGracefully(queue: .global()) { _ in } | ||
| } |
There was a problem hiding this comment.
The background shutdown helpers ignore all errors including the completion callback. This could mask critical shutdown failures in tests. Consider logging or asserting on errors during shutdown to catch potential issues during test execution.
| let pool = NIOThreadPool(numberOfThreads: 1) | ||
| pool.start() | ||
| defer { try? pool.syncShutdownGracefully() } | ||
| defer { shutdownThreadPoolInBackground(pool) } |
There was a problem hiding this comment.
Using fire-and-forget background shutdown in a defer block may not properly clean up resources before the test completes. The test may exit while shutdown is still in progress, potentially causing resource leaks or test interference. Since these tests appear to be synchronous (not async), consider whether the tests truly hang with syncShutdownGracefully, or if the hang is due to another issue that should be addressed differently.
| defer { shutdownThreadPoolInBackground(pool) } | ||
| let elg = MultiThreadedEventLoopGroup(numberOfThreads: 1) | ||
| defer { try? elg.syncShutdownGracefully() } | ||
| defer { shutdownEventLoopGroupInBackground(elg) } |
There was a problem hiding this comment.
Using fire-and-forget background shutdown in a defer block may not properly clean up resources before the test completes. The test may exit while shutdown is still in progress, potentially causing resource leaks or test interference. Since these tests appear to be synchronous (not async), consider whether the tests truly hang with syncShutdownGracefully, or if the hang is due to another issue that should be addressed differently.
| func testFromText() throws { | ||
| let elg = MultiThreadedEventLoopGroup(numberOfThreads: 1) | ||
| defer { try? elg.syncShutdownGracefully() } | ||
| defer { shutdownEventLoopGroupInBackground(elg) } |
There was a problem hiding this comment.
Using fire-and-forget background shutdown in a defer block may not properly clean up resources before the test completes. The test may exit while shutdown is still in progress, potentially causing resource leaks or test interference. Since these tests appear to be synchronous (not async), consider whether the tests truly hang with syncShutdownGracefully, or if the hang is due to another issue that should be addressed differently.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
I added an sync shutdown helpers for NIO thread pools and event loop groups