Conversation
…equest when shutdown() cancelled it The failure branch attached to the initial 'CONNECTOR_INFO' request in WebSocketConnectionGroup.handshakeComplete() resubmitted the request on any failure, cancellation included. shutdown() cancels every pending remote request with getPromise().cancel(true); PromiseImpl runs the listeners on the cancelling thread and routes CANCELLED to the onException function, so the retry ran synchronously inside shutdown()'s loop - before principalIsShuttingDown() removed the sockets - and was sent over the closing connection and registered in the group being torn down. When the initial request was the only pending entry, the ConcurrentSkipListMap iterator had already advanced past the end and the retry stayed registered and pending in the dead group. The retry is now skipped when the first request's promise is cancelled. The promise state is checked rather than the exception type because RemoteOperationRequest.createCancellationException returns a failed remote cancel's exception as-is. Fixes OpenIdentityPlatform#125
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #125
Problem
The failure branch attached to the initial
CONNECTOR_INFOrequest inWebSocketConnectionGroup.handshakeComplete()resubmitted the request on any failure, cancellation included.shutdown()cancels every pending remote request withgetPromise().cancel(true);PromiseImplruns the listeners on the cancelling thread and routesCANCELLEDto theonExceptionfunction, so the retry ran synchronously insideshutdown()'s loop overremoteRequests- beforeprincipalIsShuttingDown()removed the sockets - and was sent over the closing connection and registered in the group being torn down.Two refinements to the issue text, found while verifying it:
ConcurrentSkipListMapiterator has already advanced past the end, so the retry is not visited by the shutdown loop and stays registered and pending in the dead group. The "extraCONNECTOR_INFO+CancelOpRequeston the wire" outcome happens only when other requests are pending after it.ControlRequestis handled synchronously without aLocalRequest, so the strayCancelOpRequestonly parks apendingCancelsentry for 60 s; the cost is one needlessserializeLegacyof all connector infos and a response into a closing socket.Fix
The retry is skipped when the first request's promise is cancelled. The promise state is checked rather than the exception type because
RemoteOperationRequest.createCancellationExceptionreturns a failed remote cancel's exception as-is (e.g.ConnectorIOExceptionwhen theCancelOpRequestcould not be sent), soCancellationExceptionis not a reliable marker.Test
WebSocketConnectionGroupShutdownTest: a recording socket stub and a group subclass exposingremoteRequests; handshake +handshakeComplete()send oneControlRequest, thenprincipalIsShuttingDown(). Before the fix: a secondControlRequestis sent during shutdown and the retry stays inremoteRequests. After: oneControlRequest, oneCancelOpRequest,remoteRequestsempty.Independent of #124 (the test uses an already-sent request, so it does not depend on the promise existing before the send).