Community
Participate
Working Groups
With the dstore connection, if a password is expired, password change dialog is shown. If a user cancels the dialog, the next connect attempt does not work. When the dialog is canceled, StandardCredentialsProvider.promptForNewPassword() throws an OperationCanceledException. As the result, DStoreConnectorService._isConnecting is left "true", and the succeeding connect attempt is not processed.
DaveD - this sounds simple enough for 3.0.1, ok? I think this is minor since there is a workaround (disconnect / reconnect)?
Sounds good.
Another case where _isConnecting is left true: When a new connection is created, a user can enter a port number > 65535. It causes an IllegalArgumentException when creating a socket during connect. Then _isConnecting is left true, and further connect attempt is ignored. Since disconnect menu is not available, the connection becomes useless until a user restarts the workbench. So I changed the severity back to normal.
We've had similar issues in our commercial product some years ago, where a connection would incorrectly remain in "connecting" state and thus not be usable any more. What we did to fix this was allow the "Disconnect" operation even while in "connecting" state, but re-interpret it as a "Cancel connect" request. This could be implemented by storing the IProgressMonitor that's associated with a connect request in a place associated with the host or subsystem, and in the Disconnect operation, if still in "connecting" state, find the progress monitor and cancel it. That way, there would at least be a workaround for connections that keep hanging in connecting state for whatever reason.
Given that the connection becomes unusable in the scenario mentioned, I'm raising priority to a P2.
Altering priority for product requested bugs.
Created attachment 110851 [details] Patch fixing both problems by handling the appropriate exception
DaveM -- This is a patch to code you own. Please review. Thanks!
I'm approving the patch although the details will show in english for now. This is a rare scenario and we can fix the english when we add PII later.
Dave -- can you change your review mark to a + to agree with your comment? Its a - right now. Thanks.
Changes have been committed.
Created attachment 111503 [details] Patch improving the error reporting from ClientConnection I found two problems when reviewing the changes in ClientConnection, which are fixed in attached patch: (1) In the SSL case, catching away the IllegalArgumentException is not a good idea since it could also have been thrown in code *after* the port had successfully been created. In that case, the port would not have been closed. It's better to explicitly check the daemon port number. (2) Rather than wrapping the message into an exception that's not appropriate (because it wouldn't have a stacktrace associated, AND the original exception's message is lost!), it's better to create the ConnectionStatus object with a message right away. If you don't think that's a good idea, then at the very least initCause() should be called on the exception: Exception e2 = new IllegalArgumentException(INVALID_DAEMON_PORT_NUMBER); e2.initCause(e); e = e2; Also, UI code should be reviewed to ensure that an invalid daemon port can never be entered. Would any of the Dave's approve my patch?
I filed bug 245989 to track cleaning up the UI and the PII.
I committed my patch since our ramp-down plan doesn't require review yet: [235284][dstore] Safer error reporting for invalid daemon port number
