Skip to content

Commit

Permalink
Weaken assertion in TransportService#doStop (#88601)
Browse files Browse the repository at this point in the history
We may concurrently register a handler for a (closed) connection to a
remote node while stopping. Such a handler will be completed immediately
on the sending path if not picked up in `doStop`, but sometimes it's
picked up in `doStop` anyway. This commit weakens the assertion to
account for this case too.

Closes #88112
Closes #88459
Closes #88597
  • Loading branch information
DaveCTurner committed Jul 19, 2022
1 parent 43c4346 commit 449edcf
Showing 1 changed file with 15 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -295,15 +295,26 @@ protected void doStop() {
throw new UncheckedIOException(e);
} finally {
// The underlying transport has stopped which closed all the connections to remote nodes and hence completed all their handlers,
// but there may still be pending handlers for node-local requests since this connection is not closed. We complete them here:
// but there may still be pending handlers for node-local requests since this connection is not closed, and we may also
// (briefly) track handlers for requests which are sent concurrently with stopping even though the underlying connection is
// now closed. We complete all these outstanding handlers here:
for (final Transport.ResponseContext<?> holderToNotify : responseHandlers.prune(h -> true)) {
try {
final TransportResponseHandler<?> handler = holderToNotify.handler();
final var targetNode = holderToNotify.connection().getNode();

// Assertion only holds for TcpTransport only because other transports (used in tests) may not implement the proper
// close-connection behaviour. TODO fix this.
assert transport instanceof TcpTransport == false || targetNode.equals(localNode) : targetNode + " vs " + localNode;
assert transport instanceof TcpTransport == false
/* other transports (used in tests) may not implement the proper close-connection behaviour. TODO fix this. */
|| targetNode.equals(localNode)
/* local node connection cannot be closed so may still have pending handlers */
|| holderToNotify.connection().isClosed()
/* connections to remote nodes must be closed by this point but could still have pending handlers */
: "expected only responses for local "
+ localNode
+ " but found handler for ["
+ holderToNotify.action()
+ "] on open connection to "
+ targetNode;

final var exception = new SendRequestTransportException(
targetNode,
Expand Down

0 comments on commit 449edcf

Please sign in to comment.