From 3a4351f9b777009d8e226d26125f758861ddcbb3 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Johannes=20Dahlstr=C3=B6m?= Date: Thu, 12 Sep 2013 17:48:47 +0300 Subject: [PATCH] Ensure PushConnection is properly cleaned up on disconnect (#12226, #12522) Change-Id: I0bab199632554655ef92a624f5654852b4b157d1 --- .../AtmospherePushConnection.java | 2 - .../server/communication/PushHandler.java | 90 ++++++++----- .../vaadin/tests/push/EnableDisablePush.html | 97 ++++++++++++++ .../vaadin/tests/push/EnableDisablePush.java | 119 ++++++++++++++++++ 4 files changed, 272 insertions(+), 36 deletions(-) create mode 100644 uitest/src/com/vaadin/tests/push/EnableDisablePush.html create mode 100644 uitest/src/com/vaadin/tests/push/EnableDisablePush.java diff --git a/server/src/com/vaadin/server/communication/AtmospherePushConnection.java b/server/src/com/vaadin/server/communication/AtmospherePushConnection.java index e325550c4b..b9d4955b12 100644 --- a/server/src/com/vaadin/server/communication/AtmospherePushConnection.java +++ b/server/src/com/vaadin/server/communication/AtmospherePushConnection.java @@ -221,8 +221,6 @@ public class AtmospherePushConnection implements PushConnection { } resource.resume(); - assert !resource.getBroadcaster().getAtmosphereResources() - .contains(resource); resource = null; } diff --git a/server/src/com/vaadin/server/communication/PushHandler.java b/server/src/com/vaadin/server/communication/PushHandler.java index 1c50f79349..81dd00084d 100644 --- a/server/src/com/vaadin/server/communication/PushHandler.java +++ b/server/src/com/vaadin/server/communication/PushHandler.java @@ -173,7 +173,8 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter }; /** - * Callback used when a connection is closed by the client. + * Callback used when a connection is closed, either deliberately or because + * an error occurred. */ private final PushEventCallback disconnectCallback = new PushEventCallback() { @Override @@ -192,8 +193,7 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter if (!pushMode.isEnabled()) { /* * The client is expected to close the connection after push - * mode has been set to disabled, just clean up some stuff - * and be done with it + * mode has been set to disabled. */ getLogger().log(Level.FINER, "Connection closed for resource {0}", id); @@ -248,24 +248,17 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter } catch (ServiceException e) { getLogger().log(Level.SEVERE, "Could not get session. This should never happen", e); + return; } catch (SessionExpiredException e) { SystemMessages msg = service.getSystemMessages( ServletPortletHelper.findLocale(null, null, vaadinRequest), vaadinRequest); - try { - resource.getResponse() - .getWriter() - .write(VaadinService - .createCriticalNotificationJSON( - msg.getSessionExpiredCaption(), - msg.getSessionExpiredMessage(), - null, msg.getSessionExpiredURL())); - } catch (IOException e1) { - getLogger() - .log(Level.WARNING, - "Failed to notify client about unavailable session", - e); - } + sendNotificationAndDisconnect( + resource, + VaadinService.createCriticalNotificationJSON( + msg.getSessionExpiredCaption(), + msg.getSessionExpiredMessage(), null, + msg.getSessionExpiredURL())); return; } @@ -275,21 +268,15 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter // Sets UI.currentInstance final UI ui = service.findUI(vaadinRequest); if (ui == null) { - // This a request through an already open push connection to - // a UI which no longer exists. - resource.getResponse() - .getWriter() - .write(UidlRequestHandler.getUINotFoundErrorJSON( - service, vaadinRequest)); - // End the connection - resource.resume(); - return; + sendNotificationAndDisconnect(resource, + UidlRequestHandler.getUINotFoundErrorJSON(service, + vaadinRequest)); + } else { + callback.run(resource, ui); } - - callback.run(resource, ui); } catch (IOException e) { - getLogger().log(Level.INFO, - "An error occured while writing a push response", e); + getLogger().log(Level.WARNING, "Error writing a push response", + e); } finally { session.unlock(); } @@ -323,9 +310,10 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter AtmosphereResource resource = event.getResource(); String id = resource.uuid(); - if (event.isCancelled()) { - // Disconnected for whatever reason, handle in onDisconnect() as - // it's more reliable + if (event.isCancelled() || event.isResumedOnTimeout()) { + getLogger().log(Level.FINER, + "Cancelled connection for resource {0}", id); + disconnect(event); } else if (event.isResuming()) { // A connection that was suspended earlier was resumed (committed to // the client.) Should only happen if the transport is JSONP or @@ -364,13 +352,31 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter public void onDisconnect(AtmosphereResourceEvent event) { // Log event on trace level super.onDisconnect(event); - callWithUi(event.getResource(), disconnectCallback); + disconnect(event); + } + + @Override + public void onThrowable(AtmosphereResourceEvent event) { + getLogger().log(Level.SEVERE, "Exception in push connection", + event.throwable()); + disconnect(event); + } + + @Override + public void onResume(AtmosphereResourceEvent event) { + // Log event on trace level + super.onResume(event); + disconnect(event); } @Override public void destroy() { } + private void disconnect(AtmosphereResourceEvent event) { + callWithUi(event.getResource(), disconnectCallback); + } + /** * Sends a refresh message to the given atmosphere resource. Uses an * AtmosphereResource instead of an AtmospherePushConnection even though it @@ -396,6 +402,22 @@ public class PushHandler extends AtmosphereResourceEventListenerAdapter } } + /** + * Tries to send a critical notification to the client and close the + * connection. Does nothing if the connection is already closed. + */ + private static void sendNotificationAndDisconnect( + AtmosphereResource resource, String notificationJson) { + // TODO Implemented differently from sendRefreshAndDisconnect + try { + resource.getResponse().getWriter().write(notificationJson); + resource.resume(); + } catch (Exception e) { + getLogger().log(Level.FINEST, + "Failed to send critical notification to client", e); + } + } + private static final Logger getLogger() { return Logger.getLogger(PushHandler.class.getName()); } diff --git a/uitest/src/com/vaadin/tests/push/EnableDisablePush.html b/uitest/src/com/vaadin/tests/push/EnableDisablePush.html new file mode 100644 index 0000000000..87dfa8428f --- /dev/null +++ b/uitest/src/com/vaadin/tests/push/EnableDisablePush.html @@ -0,0 +1,97 @@ + + + + + + +EnableDisablePush + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
EnableDisablePush
open/run/EnableDisablePush?restartApplication
assertTextvaadin=runEnableDisablePush::PID_SLog_row_01. Push enabled
clickvaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VButton[0]/domChild[0]/domChild[0]
assertTextvaadin=runEnableDisablePush::PID_SLog_row_03. Push disabled
clickvaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[3]/VButton[0]/domChild[0]/domChild[0]
assertTextvaadin=runEnableDisablePush::PID_SLog_row_05. Poll enabled
clickvaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[1]/VButton[0]/domChild[0]/domChild[0]
assertTextvaadin=runEnableDisablePush::PID_SLog_row_07. Push enabled
clickvaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[2]/VButton[0]/domChild[0]/domChild[0]
assertTextvaadin=runEnableDisablePush::PID_SLog_row_09. Poll disabled
clickvaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[4]/VButton[0]/domChild[0]/domChild[0]
assertTextvaadin=runEnableDisablePush::PID_SLog_row_011. Push disabled, polling enabled
pause3500
assertTextvaadin=runEnableDisablePush::PID_SLog_row_016. Polling disabled, push enabled
clickvaadin=runEnableDisablePush::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VButton[0]/domChild[0]/domChild[0]
assertTextvaadin=runEnableDisablePush::PID_SLog_row_018. Push disabled
+ + diff --git a/uitest/src/com/vaadin/tests/push/EnableDisablePush.java b/uitest/src/com/vaadin/tests/push/EnableDisablePush.java new file mode 100644 index 0000000000..1911c66c2d --- /dev/null +++ b/uitest/src/com/vaadin/tests/push/EnableDisablePush.java @@ -0,0 +1,119 @@ +package com.vaadin.tests.push; + +import java.util.Date; +import java.util.Timer; +import java.util.TimerTask; +import java.util.concurrent.TimeUnit; + +import com.vaadin.server.VaadinRequest; +import com.vaadin.shared.communication.PushMode; +import com.vaadin.tests.components.AbstractTestUI; +import com.vaadin.tests.util.Log; +import com.vaadin.ui.Button; +import com.vaadin.ui.UIDetachedException; + +public class EnableDisablePush extends AbstractTestUI { + + private int c = 0; + + private Log log = new Log(15); + + private final Timer timer = new Timer(true); + + private final class CounterTask extends TimerTask { + + @Override + public void run() { + + try { + while (true) { + TimeUnit.MILLISECONDS.sleep(1000); + + access(new Runnable() { + @Override + public void run() { + log.log("Counter = " + c++); + if (c == 3) { + log.log("Disabling polling, enabling push"); + getPushConfiguration().setPushMode( + PushMode.AUTOMATIC); + setPollInterval(-1); + log.log("Polling disabled, push enabled"); + } + } + }); + } + } catch (InterruptedException e) { + } catch (UIDetachedException e) { + } + } + }; + + @Override + protected void setup(VaadinRequest request) { + + getPushConfiguration().setPushMode(PushMode.AUTOMATIC); + log.log("Push enabled"); + + addComponent(new Button("Disable push", new Button.ClickListener() { + @Override + public void buttonClick(Button.ClickEvent event) { + log.log("Disabling push"); + getPushConfiguration().setPushMode(PushMode.DISABLED); + log.log("Push disabled"); + } + })); + + addComponent(new Button("Enable push", new Button.ClickListener() { + @Override + public void buttonClick(Button.ClickEvent event) { + log.log("Enabling push"); + getPushConfiguration().setPushMode(PushMode.AUTOMATIC); + log.log("Push enabled"); + } + })); + + addComponent(new Button("Disable polling", new Button.ClickListener() { + @Override + public void buttonClick(Button.ClickEvent event) { + log.log("Disabling poll"); + setPollInterval(-1); + log.log("Poll disabled"); + } + })); + + addComponent(new Button("Enable polling", new Button.ClickListener() { + @Override + public void buttonClick(Button.ClickEvent event) { + log.log("Enabling poll"); + setPollInterval(1000); + log.log("Poll enabled"); + } + })); + + addComponent(new Button( + "Disable push, re-enable from background thread", + new Button.ClickListener() { + @Override + public void buttonClick(Button.ClickEvent event) { + log.log("Disabling push, enabling polling"); + getPushConfiguration().setPushMode(PushMode.DISABLED); + setPollInterval(1000); + timer.schedule(new CounterTask(), new Date()); + log.log("Push disabled, polling enabled"); + } + })); + + addComponent(log); + } + + @Override + protected String getTestDescription() { + return "Test dynamically enablind and disabling push"; + } + + @Override + protected Integer getTicketNumber() { + return 12226; + } +} -- 2.39.5