Ensure ConnectionPool.triggerForceShutdown() works and that it doesn't shutdown until all connections are closed
#607
Conversation
adam-fowler
changed the title
ConnectionPool .triggerForceShutdown() works and that it doesn't shutdown until all connections are closedConnectionPool.triggerForceShutdown() works and that it doesn't shutdown until all connections are closed
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #607 +/- ##
==========================================
+ Coverage 75.68% 75.74% +0.05%
==========================================
Files 134 134
Lines 9912 10009 +97
==========================================
+ Hits 7502 7581 +79
- Misses 2410 2428 +18
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| @available(macOS 13.0, iOS 16.0, tvOS 16.0, watchOS 9.0, *) | ||
| @Test func testForceShutdown() async throws { |
There was a problem hiding this comment.
can we add a second test that tests, that we even close leased connections?
There was a problem hiding this comment.
Added, plus I added an extra test that ensure we shutdown correctly if a connection request is in progress
| index: Int, | ||
| availableContext: ConnectionGroup.AvailableConnectionContext | ||
| availableContext: ConnectionGroup.AvailableConnectionContext, | ||
| shuttingDown: Bool |
There was a problem hiding this comment.
we should not pass around shuttingDown here and instead rely on self.state.
| case shutdown(Shutdown) | ||
|
|
||
| case initiateShutdown(Shutdown) | ||
| case shutdown([TimerCancellationToken]) |
There was a problem hiding this comment.
what is the difference between initiateShutdown and shutdown? can we add a developer comment here please?
There was a problem hiding this comment.
Wanna rename this to cancelEventStreamAndFinalCleanup?
| case .idle(_, let newIdle): | ||
| if shuttingDown { |
There was a problem hiding this comment.
can we use a case ... where here?
fabianfett
left a comment
fabianfett
left a comment
There was a problem hiding this comment.
Overall this is really, really great. Thanks so much for the improvements!
Now that we keep a record of all connections when shutting down it is possible for a connection to be released while in the closing state.
| case .backingOff, .starting, .idle, .closing, .closed: | ||
|
|
||
| case .closing: | ||
| return nil |
There was a problem hiding this comment.
Now that we keep a record of all the connections during the shutdown It is now possible to catch a connection in the closing state when release is called after triggerForceShutdown
2 participants
This PR fixes
ConnectionPool.triggerForceShutdown()so that it actually shuts down the connection pool and therun()method returns.I renamed the
shutdownconnection pool action toinitiateShutdownand added a new actionshutdown. TheinitiateShutdownaction will close all the connections and related timers and is returned bytriggerForceShutdown. Theshutdownaction will finish the eventStream thus allowing therunmethod to return. TheconnectionClosedmethod will return theshutdownaction once the connections array is empty. There is also support for dealing with connections that are established aftertriggerForceShutdownis called which wasn't there before.I have also added tests for the state machine, for when connections are idle, leased or a connection request is in process. And a single ConnectionPool test that checks
triggerForceShutdownactually causes therunmethod to return.Ensure
ConnectionPool.triggerForceShutdownworks and that it doesn't shutdown until all connections are closed