Make Subchannel internal state handling clearer (grpc#1949)

Motivation: The subchannel conflates closing (recoverable) with shutting down (terminal). Shutting down is initiated by a higher level (load-balancer, user) while closing happens when the subchannel closes unexpectedtly or is no longer required (i.e. becomes idle). A closed subchannel can be re-opeend, a shutdown subchannel can't. This distinction isn't clear enough in the state handling. Modifications: - Rename 'close' to 'shutDown' where applicable - Add a new 'shutting down' state and renaming the 'closing' state to 'going away'. - Add a state machine diagram - Fix up a few state transitions Result: More robust state handling
hharng · Jun 24, 2024 · 621bcf2 · 621bcf2
1 parent 82447fc
commit 621bcf2
Show file tree

Hide file tree

Showing 5 changed files with 164 additions and 120 deletions.
diff --git a/Sources/GRPCHTTP2Core/Client/Connection/LoadBalancers/PickFirstLoadBalancer.swift b/Sources/GRPCHTTP2Core/Client/Connection/LoadBalancers/PickFirstLoadBalancer.swift
@@ -185,7 185,7 @@ extension PickFirstLoadBalancer {
  switch onUpdate {
  case .connect(let newSubchannel, close: let oldSubchannel):
  self.runSubchannel(newSubchannel, in: &group)
- oldSubchannel?.close()
  oldSubchannel?.shutDown()
 
  case .none:
  ()
@@ -226,9 226,9 @@ extension PickFirstLoadBalancer {
 
  switch onUpdateState {
  case .close(let subchannel):
- subchannel.close()
  subchannel.shutDown()
  case .closeAndPublishStateChange(let subchannel, let connectivityState):
- subchannel.close()
  subchannel.shutDown()
  self.event.continuation.yield(.connectivityStateChanged(connectivityState))
  case .publishStateChange(let connectivityState):
  self.event.continuation.yield(.connectivityStateChanged(connectivityState))
@@ -251,8 251,8 @@ extension PickFirstLoadBalancer {
  switch onClose {
  case .closeSubchannels(let subchannel1, let subchannel2):
  self.event.continuation.yield(.connectivityStateChanged(.shutdown))
- subchannel1.close()
- subchannel2?.close()
  subchannel1.shutDown()
  subchannel2?.shutDown()
 
  case .closed:
  self.event.continuation.yield(.connectivityStateChanged(.shutdown))

diff --git a/Sources/GRPCHTTP2Core/Client/Connection/LoadBalancers/RoundRobinLoadBalancer.swift b/Sources/GRPCHTTP2Core/Client/Connection/LoadBalancers/RoundRobinLoadBalancer.swift
@@ -245,7 245,7 @@ extension RoundRobinLoadBalancer {
  // present if there are more to remove than to add. These are the excess subchannels which
  // are closed now.
  for subchannel in removed {
- subchannel.close()
  subchannel.shutDown()
  }
  }
 
@@ -288,10 288,10 @@ extension RoundRobinLoadBalancer {
 
  case .closeAndPublishStateChange(let subchannel, let aggregateState):
  self.event.continuation.yield(.connectivityStateChanged(aggregateState))
- subchannel.close()
  subchannel.shutDown()
 
  case .close(let subchannel):
- subchannel.close()
  subchannel.shutDown()
 
  case .closed:
  // All subchannels are closed; finish the streams so the run loop exits.
@@ -306,7 306,7 @@ extension RoundRobinLoadBalancer {
  private func handleSubchannelGoingAway(key: EndpointKey) {
  switch self.state.withLockedValue({ $0.parkSubchannel(withKey: key) }) {
  case .closeAndUpdateState(let subchannel, let connectivityState):
- subchannel.close()
  subchannel.shutDown()
  if let connectivityState = connectivityState {
  self.event.continuation.yield(.connectivityStateChanged(connectivityState))
  }
@@ -323,7 323,7 @@ extension RoundRobinLoadBalancer {
 
  // Close the subchannels.
  for subchannel in subchannels {
- subchannel.close()
  subchannel.shutDown()
  }
 
  case .closed: