chore: Don't use synchronous methods in asynchronous contexts #56
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors resource shutdown to use synchronous helper methods in defer blocks and updates CI workflows for multiple Swift versions and adjusted timeouts.
- Introduces
syncShutdown()methods inFlightServerImplandFlightClientTesterand replaces inline shutdown calls with defer blocks - Removes the
continue-on-erroroverride from the test workflow - Adds a Swift version matrix and reduces the job timeout in the release candidate workflow
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ArrowFlight/Tests/ArrowFlightTests/FlightTest.swift | Added syncShutdown helpers and updated test defer blocks for cleanup |
| .github/workflows/test.yaml | Removed deprecated continue-on-error setting |
| .github/workflows/rc.yaml | Reduced timeout-minutes and introduced Swift version matrix |
Comments suppressed due to low confidence (1)
.github/workflows/rc.yaml:108
- [nitpick] A 15-minute timeout may be insufficient for matrix builds on slower runners. Consider increasing the timeout to avoid CI flakiness.
timeout-minutes: 15
| try? group?.syncShutdownGracefully() | ||
| try? channel?.close().wait() | ||
| func syncShutdown() throws { | ||
| try group?.syncShutdownGracefully() |
There was a problem hiding this comment.
The instance property 'group' is never assigned in the initializer, so group remains nil and this shutdown call is a no-op. Consider assigning self.group = group in init() to ensure proper shutdown.
There was a problem hiding this comment.
well this was not helpful, it is initialized on init at line 176
let group = PlatformSupport.makeEventLoopGroup(loopCount: 1)
raulcd
left a comment
There was a problem hiding this comment.
LGTM, I don't know much about swift but it fixes the issue and it runs on the different versions. @dongjoon-hyun do you want to take a quick look?
| try? group?.syncShutdownGracefully() | ||
| try? channel?.close().wait() | ||
| func syncShutdown() throws { | ||
| try group?.syncShutdownGracefully() |
There was a problem hiding this comment.
well this was not helpful, it is initialized on init at line 176
let group = PlatformSupport.makeEventLoopGroup(loopCount: 1)
What's Changed
Creates synchronous methods and call it in
deferto free resources.Closes #55.