Review ClientUpgradeRequest exception handling

[sbordet]

Jetty 10.0.x's org.eclipse.jetty.websocket.core.client.ClientUpgradeRequest.onComplete() handles upgrade failures differently from 9.4.x.

In particular, java.net.ConnectionException should not be wrapped in an UpgradeException, so that client applications will know that the failure may be temporary (e.g. server restarting).

I would revert this logic to be the same as 9.4.x unless there are strong reasons for not to.

[joakime]

The example java.net.ConnectException is based on java.net.SocketException which is specifically excluded and not wrapped in an UpgradeException.

https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/client/ClientUpgradeRequest.java#L226-L239

Same section on jetty-9.4.x btw (which has a shorter list of excluded exception types)

https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketUpgradeRequest.java#L510-L520

[sbordet]

@joakime you pointed out that the logic is different, like I said.

Furthermore, in 9.4.x a ConnectException was notified to the WebSocket endpoint via a call to onError() (depending on whether the endpoint was implementing WebSocketListener or annotating methods) , while in 10.0.x this is not done, relying entirely on the future returned by connect().
I think this may be open to discussion, but I think the endpoint should be notified - it acts as a "callback" for WebSocket events and in this case an error trying to connect to the server.

[sbordet]

@lachlan-roberts this issue is the last one that holds back CometD 6 and therefore a release of Jetty 10, so please give it higher priority.

Some thoughts: the FrameHandler is being created really late, and only when the upgrade is successful.
I think it should be created earlier because what it does (building the method handles to invoke the WebSocket endpoint) is necessary for both the case of a successful upgrade and the case of any failure (from not even able to connect to the server to a non 101 response).
I think this needs some refactoring to various components, to receive the 'FrameHandler` reference to invoke the error methods on it.

[gregw]

Note that I think the onOpen and onError callbacks should be the primary mechanism for communicating success or failure of a connect call. I think the connect methods returning futures could be deprecated if need be... or discouraged in some way.

[lachlan-roberts]

Currently jetty-10.0.x uses onError quite differently to how it was used in jetty-9.4.x.

I believe in jetty-9.4.x onError can be called multiple times and you can even receive an onError after you have received an onClose.

In jetty-10.0.x onError is tied directly to onClose, such that if there is an error there is also a corresponding close. So if there was no onOpen notification you will not be getting an onError or onClose call.