Fix crash in PoolStateMachine+ConnectionGroup when closing connection while keepAlive is running #444
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #444 +/- ##
==========================================
+ Coverage 59.72% 59.82% +0.10%
==========================================
Files 124 124
Lines 9889 9915 +26
==========================================
+ Hits 5906 5932 +26
Misses 3983 3983
|
fabianfett
added
semver-patch
| @@ -449,6 +449,21 @@ extension PoolStateMachine { | |||
| return (index, context) | |||
| } | |||
|
|
|||
| /// Returns `true` if the connection should be explicitly closed, `false` if nothing needs to be done. | |||
| @inlinable | |||
| mutating func keepAliveFailed(_ connectionID: Connection.ID) -> Bool { | |||
There was a problem hiding this comment.
Can we return an enum for this? KeepAliveFailedAction with two cases: close and none? Makes this more explicit
There was a problem hiding this comment.
This is now returning an optional CloseAction. It should be explicit enough now, let me know if not.
| mutating func keepAliveFailed(_ connectionID: Connection.ID) -> Bool { | ||
| // We don't have to inform the ConnectionStates about any of this, because the connection will close | ||
| // immediately after this or is closed already | ||
| switch self.connections.first(where: { $0.id == connectionID })?.state { |
There was a problem hiding this comment.
please don‘t access the connection‘s state here. the state is only visible here, to allow inlining. Instead please forward a call keepAliveFailed into the ConnectionStateMachine. Also make sure to change the state to closing, if we need to explicitly close the connection.
There was a problem hiding this comment.
The connection state is receiving the failure now and changes the state accordingly.
Sources/ConnectionPoolModule/PoolStateMachine+ConnectionGroup.swift
Outdated
Show resolved
Hide resolved
| @inlinable | ||
| mutating func connectionKeepAliveFailed(_ connection: Connection) -> Action { | ||
| if self.connections.keepAliveFailed(connection.id) { | ||
| return self.connectionClosed(connection) |
There was a problem hiding this comment.
That doesn‘t look right, if we the keep alive failed, we can‘t declare the connection dead right away, we need to create a close action. Also if we close and have waiting requests, we might need to create a new connection right away.
There was a problem hiding this comment.
The connection is now properly closed by reusing the existing closed states and actions. I've added a test to ensure a new connection is established if required.
…lose actions if needed
|
Thanks @lovetodream. Great catch and great fix! |
Fixes: #443