From 5fd625a945862e6ceccbeb13cde1ef00e26d9572 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Wed, 25 Feb 2015 08:06:28 +0000 Subject: [PATCH] Breaks PushLargeDataLongPollingTest and also long polling in many ways. See http://dev.vaadin.com/ticket/16919 Revert "Fix handling of disconnection with push (#15330)" This reverts commit 3c07368cbbc4d35534e90c769ea8ec975400c452. Change-Id: I46631b1921fa1c5628952362a93a000df92c5a4a --- .../AtmospherePushConnection.java | 61 ++++++------------- .../server/communication/PushHandler.java | 32 ++++------ 2 files changed, 30 insertions(+), 63 deletions(-) diff --git a/server/src/com/vaadin/server/communication/AtmospherePushConnection.java b/server/src/com/vaadin/server/communication/AtmospherePushConnection.java index aa952b90724..0819a24ee95 100644 --- a/server/src/com/vaadin/server/communication/AtmospherePushConnection.java +++ b/server/src/com/vaadin/server/communication/AtmospherePushConnection.java @@ -156,9 +156,9 @@ public void push() { public void push(boolean async) { if (!isConnected()) { if (async && state != State.RESPONSE_PENDING) { - setState(State.PUSH_PENDING); + state = State.PUSH_PENDING; } else { - setState(State.RESPONSE_PENDING); + state = State.RESPONSE_PENDING; } } else { try { @@ -229,22 +229,23 @@ public boolean isConnected() { /** * Associates this {@code AtmospherePushConnection} with the given * {@AtmosphereResource} representing an established - * push connection. - *

+ * push connection. If already connected, calls {@link #disconnect()} first. * If there is a deferred push, carries it out via the new connection. - *

- * This method must never be called when there is an existing connection * * @since 7.2 */ public void connect(AtmosphereResource resource) { + assert resource != null; assert resource != this.resource; - assert !isConnected(); + + if (isConnected()) { + disconnect(); + } this.resource = resource; State oldState = state; - setState(State.CONNECTED); + state = State.CONNECTED; if (oldState == State.PUSH_PENDING || oldState == State.RESPONSE_PENDING) { @@ -272,17 +273,14 @@ protected AtmosphereResource getResource() { @Override public void disconnect() { assert isConnected(); - if (!isConnected() || resource.isResumed()) { - // Server has asked to push connection to be disconnected but there - // is no current connection - - // This could be a timing issue during session expiration or - // similar and should not happen in an ideal world. Might require - // some logic changes in session expiration handling to avoid - // triggering this - getLogger().info( - "Server requested disconnect for inactive push connection"); - connectionLost(); + + if (resource.isResumed()) { + // Calling disconnect may end up invoking it again via + // resource.resume and PushHandler.onResume. Bail out here if + // the resource is already resumed; this is a bit hacky and should + // be implemented in a better way in 7.2. + resource = null; + state = State.DISCONNECTED; return; } @@ -304,24 +302,13 @@ public void disconnect() { } try { - // Close the push connection resource.close(); } catch (IOException e) { getLogger() .log(Level.INFO, "Error when closing push connection", e); } - connectionLost(); - } - - /** - * Called when the connection to the client has been lost. - * - * @since - */ - public void connectionLost() { resource = null; - setState(State.DISCONNECTED); - + state = State.DISCONNECTED; } /** @@ -331,16 +318,6 @@ protected State getState() { return state; } - /** - * Sets the state of this connection - * - * @param state - * the new state - */ - private void setState(State state) { - this.state = state; - } - /** * Reinitializes this PushConnection after deserialization. The connection * is initially in disconnected state; the client will handle the @@ -349,7 +326,7 @@ private void setState(State state) { private void readObject(ObjectInputStream stream) throws IOException, ClassNotFoundException { stream.defaultReadObject(); - setState(State.DISCONNECTED); + state = State.DISCONNECTED; } private static Logger getLogger() { diff --git a/server/src/com/vaadin/server/communication/PushHandler.java b/server/src/com/vaadin/server/communication/PushHandler.java index 38a5f3017fe..7e7183193a3 100644 --- a/server/src/com/vaadin/server/communication/PushHandler.java +++ b/server/src/com/vaadin/server/communication/PushHandler.java @@ -63,9 +63,8 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter { public void onStateChange(AtmosphereResourceEvent event) throws IOException { super.onStateChange(event); - - if (event.isResumedOnTimeout()) { - connectionLost(event); + if (event.isCancelled() || event.isResumedOnTimeout()) { + disconnect(event); } } @@ -330,30 +329,17 @@ private static AtmospherePushConnection getConnectionForUI(UI ui) { public void onDisconnect(AtmosphereResourceEvent event) { // Log event on trace level super.onDisconnect(event); - - // Called no matter if the client closed the connection cleanly - // (event.isClosedByClient()) or if it was unexpectedly disconnected - // (event.isCancelled) - connectionLost(event); - } - - @Override - public void onResume(AtmosphereResourceEvent event) { - super.onResume(event); - - // If a long polling connection is resumed, we no longer have a - // connection up and must update the state of AtmospherePushConnection - connectionLost(event); + disconnect(event); } @Override public void onThrowable(AtmosphereResourceEvent event) { getLogger().log(Level.SEVERE, "Exception in push connection", event.throwable()); - connectionLost(event); + disconnect(event); } - private void connectionLost(AtmosphereResourceEvent event) { + private void disconnect(AtmosphereResourceEvent event) { // We don't want to use callWithUi here, as it assumes there's a client // request active and does requestStart and requestEnd among other // things. @@ -439,8 +425,12 @@ private void connectionLost(AtmosphereResourceEvent event) { "Connection unexpectedly closed for resource {0} with transport {1}", new Object[] { id, resource.transport() }); } - - pushConnection.connectionLost(); + if (pushConnection.isConnected()) { + // disconnect() assumes the push connection is connected but + // this method can currently be called more than once during + // disconnect, depending on the situation + pushConnection.disconnect(); + } } } catch (final Exception e) {