From 9b93969613392d5868116f8ebd15e8367cb9ef9e Mon Sep 17 00:00:00 2001 From: Joonas Lehtinen Date: Wed, 17 Sep 2008 16:21:31 +0000 Subject: [PATCH] Fixes #1970 : Need for a safe way to use application from multiple windows without synchronization errors This is rather complex new feature that has a high risk of regressions. svn changeset:5425/svn branch:trunk --- src/com/itmill/toolkit/Application.java | 60 ++- .../gwt/client/ApplicationConnection.java | 17 + .../toolkit/terminal/gwt/client/ui/IView.java | 19 +- .../gwt/server/ApplicationServlet.java | 3 + .../gwt/server/CommunicationManager.java | 431 ++++++++++-------- .../toolkit/tests/tickets/Ticket1970.java | 39 +- 6 files changed, 362 insertions(+), 207 deletions(-) diff --git a/src/com/itmill/toolkit/Application.java b/src/com/itmill/toolkit/Application.java index 6296319498..c8f318979d 100644 --- a/src/com/itmill/toolkit/Application.java +++ b/src/com/itmill/toolkit/Application.java @@ -191,7 +191,7 @@ public abstract class Application implements URIHandler, Terminal.ErrorListener *

* *

- * Since version 5.0 all windows can be referenced by their names in url + * All windows can be referenced by their names in url * http://host:port/foo/bar/ where * http://host:port/foo/ is the application url as returned by * getURL() and bar is the name of the window. @@ -204,10 +204,34 @@ public abstract class Application implements URIHandler, Terminal.ErrorListener *

* *

+ * If for some reason user opens another window with same url that is + * already open, name is modified by adding "_12345678" postfix to the name, + * where 12345678 is a random number. One can decide to create another + * window-object for those windows (recommended) or to discard the postfix. + * If the user has two browser windows pointing to the same window-object on + * server, synchronization errors are likely to occur. + *

+ * + *

+ * If no browser-level windowing is used, all defaults are fine and this + * method can be left as is. In case browser-level windows are needed, it is + * recommended to create new window-objects on this method from their names + * if the super.getWindow() does not find existing windows. See below for + * implementation example: + * // If we already have the requested window, use it + Window w = super.getWindow(name); + if (w == null) { + + // If no window found, create it + w = createNewWindow(name); + } + return w; + *

+ * + *

* The method should return null if the window does not exists (and is not - * created as a side-effect) or if the application is not running anymore + * created as a side-effect) or if the application is not running anymore. *

- * . * * @param name * the name of the window. @@ -682,24 +706,26 @@ public abstract class Application implements URIHandler, Terminal.ErrorListener } // If the uri is in some window, handle the window uri - Window window = getWindow(prefix); - if (window != null) { - URL windowContext; - try { - windowContext = new URL(context, prefix + "/"); - final String windowUri = relativeUri.length() > prefix.length() + 1 ? relativeUri - .substring(prefix.length() + 1) - : ""; - return window.handleURI(windowContext, windowUri); - } catch (final MalformedURLException e) { - terminalError(new ApplicationError(e)); - return null; + if (prefix != null && prefix.length() > 0) { + Window window = getWindow(prefix); + if (window != null) { + URL windowContext; + try { + windowContext = new URL(context, prefix + "/"); + final String windowUri = relativeUri.length() > prefix + .length() + 1 ? relativeUri.substring(prefix + .length() + 1) : ""; + return window.handleURI(windowContext, windowUri); + } catch (final MalformedURLException e) { + terminalError(new ApplicationError(e)); + return null; + } } } // If the uri was not pointing to a window, handle the // uri in main window - window = getMainWindow(); + Window window = getMainWindow(); if (window != null) { return window.handleURI(context, relativeUri); } @@ -1484,7 +1510,7 @@ public abstract class Application implements URIHandler, Terminal.ErrorListener public class ApplicationError implements Terminal.ErrorEvent { - private Throwable throwable; + private final Throwable throwable; public ApplicationError(Throwable throwable) { this.throwable = throwable; diff --git a/src/com/itmill/toolkit/terminal/gwt/client/ApplicationConnection.java b/src/com/itmill/toolkit/terminal/gwt/client/ApplicationConnection.java index 006c9682b2..c6a45ed2b4 100755 --- a/src/com/itmill/toolkit/terminal/gwt/client/ApplicationConnection.java +++ b/src/com/itmill/toolkit/terminal/gwt/client/ApplicationConnection.java @@ -250,6 +250,9 @@ public class ApplicationConnection { if (repaintAll) { uri += "?repaintAll=1"; } + if (windowName != null && windowName.length() > 0) { + uri += (repaintAll ? "&" : "?") + "windowName=" + windowName; + } if (!forceSync) { final RequestBuilder rb = new RequestBuilder(RequestBuilder.POST, @@ -1087,4 +1090,18 @@ public class ApplicationConnection { public void requestLayoutPhase() { layoutTimer.schedule(500); } + + private String windowName = null; + + /** + * Reset the name of the current browser-window. This should reflect the + * window-name used in the server, but might be different from the + * window-object target-name on client. + * + * @param stringAttribute + * New name for the window. + */ + public void setWindowName(String newName) { + windowName = newName; + } } diff --git a/src/com/itmill/toolkit/terminal/gwt/client/ui/IView.java b/src/com/itmill/toolkit/terminal/gwt/client/ui/IView.java index f878a9bacb..3611e6e0d5 100644 --- a/src/com/itmill/toolkit/terminal/gwt/client/ui/IView.java +++ b/src/com/itmill/toolkit/terminal/gwt/client/ui/IView.java @@ -67,9 +67,6 @@ public class IView extends SimplePanel implements Paintable, RootPanel.get(elementId).add(this); - Window.addWindowResizeListener(this); - Window.addWindowCloseListener(this); - // set focus to iview element by default to listen possible keyboard // shortcuts if (BrowserInfo.get().isOpera() || BrowserInfo.get().isSafari() @@ -116,6 +113,7 @@ public class IView extends SimplePanel implements Paintable, public void updateFromUIDL(UIDL uidl, ApplicationConnection client) { id = uidl.getId(); + boolean firstPaint = connection == null; connection = client; String newTheme = uidl.getStringAttribute("theme"); @@ -130,6 +128,10 @@ public class IView extends SimplePanel implements Paintable, addStyleName(uidl.getStringAttribute("style")); } + if (uidl.hasAttribute("name")) { + client.setWindowName(uidl.getStringAttribute("name")); + } + com.google.gwt.user.client.Window.setTitle(uidl .getStringAttribute("caption")); @@ -228,6 +230,13 @@ public class IView extends SimplePanel implements Paintable, w.hide(); } + // Add window listeners on first paint, to prevent premature + // variablechanges + if (firstPaint) { + Window.addWindowCloseListener(this); + Window.addWindowResizeListener(this); + } + onWindowResized(Window.getClientWidth(), Window.getClientHeight()); // IE somehow fails some layout on first run, force layout // functions @@ -317,8 +326,8 @@ public class IView extends SimplePanel implements Paintable, } private static native void focusElement(Element e) /*-{ - e.focus(); - }-*/; + e.focus(); + }-*/; public String onWindowClosing() { return null; diff --git a/src/com/itmill/toolkit/terminal/gwt/server/ApplicationServlet.java b/src/com/itmill/toolkit/terminal/gwt/server/ApplicationServlet.java index 939977b8db..c4471ac11e 100644 --- a/src/com/itmill/toolkit/terminal/gwt/server/ApplicationServlet.java +++ b/src/com/itmill/toolkit/terminal/gwt/server/ApplicationServlet.java @@ -1458,6 +1458,9 @@ public class ApplicationServlet extends HttpServlet { // Finds the window where the request is handled String path = request.getPathInfo(); + if (isApplicationRunnerServlet) { + path = path.substring(1 + applicationRunnerClassname.length()); + } // Main window as the URI is empty if (path == null || path.length() == 0 || path.equals("/")) { diff --git a/src/com/itmill/toolkit/terminal/gwt/server/CommunicationManager.java b/src/com/itmill/toolkit/terminal/gwt/server/CommunicationManager.java index 5da4bfa95b..22d00251d5 100644 --- a/src/com/itmill/toolkit/terminal/gwt/server/CommunicationManager.java +++ b/src/com/itmill/toolkit/terminal/gwt/server/CommunicationManager.java @@ -80,6 +80,8 @@ public class CommunicationManager implements Paintable.RepaintRequestListener { public static final String VAR_BURST_SEPARATOR = "\u001d"; + private final HashSet currentlyOpenWindowsInClient = new HashSet(); + private static final int MAX_BUFFER_SIZE = 64 * 1024; private final ArrayList dirtyPaintabletSet = new ArrayList(); @@ -94,6 +96,10 @@ public class CommunicationManager implements Paintable.RepaintRequestListener { private final Application application; + // Note that this is only accessed from synchronized block and + // thus should be thread-safe. + private String closingWindowName = null; + private List locales; private int pendingLocalesIndex; @@ -225,7 +231,7 @@ public class CommunicationManager implements Paintable.RepaintRequestListener { // Finds the window within the application Window window = null; if (application.isRunning()) { - window = getApplicationWindow(request, application); + window = getApplicationWindow(request, application, null); // Returns if no window found if (window == null) { // This should not happen, no windows exists but @@ -274,6 +280,13 @@ public class CommunicationManager implements Paintable.RepaintRequestListener { paintAfterVariablechanges(request, response, applicationServlet, repaintAll, outWriter, window); + + // Mark this window to be open on client + currentlyOpenWindowsInClient.add(window.getName()); + if (closingWindowName != null) { + currentlyOpenWindowsInClient.remove(closingWindowName); + closingWindowName = null; + } } out.flush(); @@ -311,188 +324,197 @@ public class CommunicationManager implements Paintable.RepaintRequestListener { outWriter.print("\"changes\":["); - // re-get mainwindow - may have been changed - Window newWindow = getApplicationWindow(request, application); - if (newWindow != window) { - window = newWindow; - repaintAll = true; - } + // If the browser-window has been closed - we do not need to paint it at + // all + if (!window.getName().equals(closingWindowName)) { - JsonPaintTarget paintTarget = new JsonPaintTarget(this, outWriter, - !repaintAll); + // re-get window - may have been changed + Window newWindow = getApplicationWindow(request, application, + window); + if (newWindow != window) { + window = newWindow; + repaintAll = true; + } - // Paints components - ArrayList paintables; - if (repaintAll) { - paintables = new ArrayList(); - paintables.add(window); + JsonPaintTarget paintTarget = new JsonPaintTarget(this, outWriter, + !repaintAll); - // Reset sent locales - locales = null; - requireLocale(application.getLocale().toString()); + // Paints components + ArrayList paintables; + if (repaintAll) { + paintables = new ArrayList(); + paintables.add(window); - } else { - // remove detached components from paintableIdMap so they - // can be GC'ed - for (Iterator it = paintableIdMap.keySet().iterator(); it.hasNext();) { - Component p = (Component) it.next(); - if (p.getApplication() == null) { - idPaintableMap.remove(paintableIdMap.get(p)); - it.remove(); - dirtyPaintabletSet.remove(p); - p.removeListener(this); + // Reset sent locales + locales = null; + requireLocale(application.getLocale().toString()); + + } else { + // remove detached components from paintableIdMap so they + // can be GC'ed + for (Iterator it = paintableIdMap.keySet().iterator(); it + .hasNext();) { + Component p = (Component) it.next(); + if (p.getApplication() == null) { + idPaintableMap.remove(paintableIdMap.get(p)); + it.remove(); + dirtyPaintabletSet.remove(p); + p.removeListener(this); + } } + paintables = getDirtyComponents(window); } - paintables = getDirtyComponents(window); - } - if (paintables != null) { - - // We need to avoid painting children before parent. - // This is ensured by ordering list by depth in component - // tree - Collections.sort(paintables, new Comparator() { - public int compare(Object o1, Object o2) { - Component c1 = (Component) o1; - Component c2 = (Component) o2; - int d1 = 0; - while (c1.getParent() != null) { - d1++; - c1 = c1.getParent(); - } - int d2 = 0; - while (c2.getParent() != null) { - d2++; - c2 = c2.getParent(); - } - if (d1 < d2) { - return -1; - } - if (d1 > d2) { - return 1; + if (paintables != null) { + + // We need to avoid painting children before parent. + // This is ensured by ordering list by depth in component + // tree + Collections.sort(paintables, new Comparator() { + public int compare(Object o1, Object o2) { + Component c1 = (Component) o1; + Component c2 = (Component) o2; + int d1 = 0; + while (c1.getParent() != null) { + d1++; + c1 = c1.getParent(); + } + int d2 = 0; + while (c2.getParent() != null) { + d2++; + c2 = c2.getParent(); + } + if (d1 < d2) { + return -1; + } + if (d1 > d2) { + return 1; + } + return 0; } - return 0; - } - }); + }); - for (final Iterator i = paintables.iterator(); i.hasNext();) { - final Paintable p = (Paintable) i.next(); + for (final Iterator i = paintables.iterator(); i.hasNext();) { + final Paintable p = (Paintable) i.next(); - // TODO CLEAN - if (p instanceof Window) { - final Window w = (Window) p; - if (w.getTerminal() == null) { - w - .setTerminal(application.getMainWindow() - .getTerminal()); + // TODO CLEAN + if (p instanceof Window) { + final Window w = (Window) p; + if (w.getTerminal() == null) { + w.setTerminal(application.getMainWindow() + .getTerminal()); + } } + /* + * This does not seem to happen in tk5, but remember this + * case: else if (p instanceof Component) { if (((Component) + * p).getParent() == null || ((Component) + * p).getApplication() == null) { // Component requested + * repaint, but is no // longer attached: skip + * paintablePainted(p); continue; } } + */ + + // TODO we may still get changes that have been + // rendered already (changes with only cached flag) + if (paintTarget.needsToBePainted(p)) { + paintTarget.startTag("change"); + paintTarget.addAttribute("format", "uidl"); + final String pid = getPaintableId(p); + paintTarget.addAttribute("pid", pid); + + p.paint(paintTarget); + + paintTarget.endTag("change"); + } + paintablePainted(p); } - /* - * This does not seem to happen in tk5, but remember this case: - * else if (p instanceof Component) { if (((Component) - * p).getParent() == null || ((Component) p).getApplication() == - * null) { // Component requested repaint, but is no // longer - * attached: skip paintablePainted(p); continue; } } - */ - - // TODO we may still get changes that have been - // rendered already (changes with only cached flag) - if (paintTarget.needsToBePainted(p)) { - paintTarget.startTag("change"); - paintTarget.addAttribute("format", "uidl"); - final String pid = getPaintableId(p); - paintTarget.addAttribute("pid", pid); - - p.paint(paintTarget); - - paintTarget.endTag("change"); - } - paintablePainted(p); } - } - paintTarget.close(); - outWriter.print("]"); // close changes + paintTarget.close(); + outWriter.print("]"); // close changes - outWriter.print(", \"meta\" : {"); - boolean metaOpen = false; + outWriter.print(", \"meta\" : {"); + boolean metaOpen = false; - if (repaintAll) { - metaOpen = true; - outWriter.write("\"repaintAll\":true"); - } + if (repaintAll) { + metaOpen = true; + outWriter.write("\"repaintAll\":true"); + } - // add meta instruction for client to set focus if it is set - final Paintable f = (Paintable) application.consumeFocus(); - if (f != null) { - if (metaOpen) { - outWriter.write(","); + // add meta instruction for client to set focus if it is set + final Paintable f = (Paintable) application.consumeFocus(); + if (f != null) { + if (metaOpen) { + outWriter.write(","); + } + outWriter.write("\"focus\":\"" + getPaintableId(f) + "\""); } - outWriter.write("\"focus\":\"" + getPaintableId(f) + "\""); - } - outWriter.print("}, \"resources\" : {"); + outWriter.print("}, \"resources\" : {"); - // Precache custom layouts - String themeName = window.getTheme(); - if (request.getParameter("theme") != null) { - themeName = request.getParameter("theme"); - } - if (themeName == null) { - themeName = "default"; - } - - // TODO We should only precache the layouts that are not - // cached already - int resourceIndex = 0; - for (final Iterator i = paintTarget.getPreCachedResources().iterator(); i - .hasNext();) { - final String resource = (String) i.next(); - InputStream is = null; - try { - is = applicationServlet.getServletContext() - .getResourceAsStream( - "/" + ApplicationServlet.THEME_DIRECTORY_PATH - + themeName + "/" + resource); - } catch (final Exception e) { - // FIXME: Handle exception - e.printStackTrace(); + // Precache custom layouts + String themeName = window.getTheme(); + if (request.getParameter("theme") != null) { + themeName = request.getParameter("theme"); + } + if (themeName == null) { + themeName = "default"; } - if (is != null) { - - outWriter.print((resourceIndex++ > 0 ? ", " : "") + "\"" - + resource + "\" : "); - final StringBuffer layout = new StringBuffer(); + // TODO We should only precache the layouts that are not + // cached already + int resourceIndex = 0; + for (final Iterator i = paintTarget.getPreCachedResources() + .iterator(); i.hasNext();) { + final String resource = (String) i.next(); + InputStream is = null; try { - final InputStreamReader r = new InputStreamReader(is, - "UTF-8"); - final char[] buffer = new char[20000]; - int charsRead = 0; - while ((charsRead = r.read(buffer)) > 0) { - layout.append(buffer, 0, charsRead); + is = applicationServlet + .getServletContext() + .getResourceAsStream( + "/" + + ApplicationServlet.THEME_DIRECTORY_PATH + + themeName + "/" + resource); + } catch (final Exception e) { + // FIXME: Handle exception + e.printStackTrace(); + } + if (is != null) { + + outWriter.print((resourceIndex++ > 0 ? ", " : "") + "\"" + + resource + "\" : "); + final StringBuffer layout = new StringBuffer(); + + try { + final InputStreamReader r = new InputStreamReader(is, + "UTF-8"); + final char[] buffer = new char[20000]; + int charsRead = 0; + while ((charsRead = r.read(buffer)) > 0) { + layout.append(buffer, 0, charsRead); + } + r.close(); + } catch (final java.io.IOException e) { + // FIXME: Handle exception + System.err.println("Resource transfer failed: " + + request.getRequestURI() + ". (" + + e.getMessage() + ")"); } - r.close(); - } catch (final java.io.IOException e) { + outWriter.print("\"" + + JsonPaintTarget.escapeJSON(layout.toString()) + + "\""); + } else { // FIXME: Handle exception - System.err.println("Resource transfer failed: " - + request.getRequestURI() + ". (" + e.getMessage() - + ")"); + System.err.println("CustomLayout " + "/" + + ApplicationServlet.THEME_DIRECTORY_PATH + + themeName + "/" + resource + " not found!"); } - outWriter.print("\"" - + JsonPaintTarget.escapeJSON(layout.toString()) + "\""); - } else { - // FIXME: Handle exception - System.err.println("CustomLayout " + "/" - + ApplicationServlet.THEME_DIRECTORY_PATH + themeName - + "/" + resource + " not found!"); } - } - outWriter.print("}"); - - printLocaleDeclarations(outWriter); + outWriter.print("}"); - outWriter.print("}]"); + printLocaleDeclarations(outWriter); + outWriter.print("}]"); + } outWriter.flush(); outWriter.close(); } @@ -581,6 +603,18 @@ public class CommunicationManager implements Paintable.RepaintRequestListener { } try { owner.changeVariables(request, m); + + // Special-case of closing browser-level + // windows: track browser-windows currently open in + // client + if (owner instanceof Window + && ((Window) owner).getParent() == null) { + final Boolean close = (Boolean) m.get("close"); + if (close != null && close.booleanValue()) { + closingWindowName = ((Window) owner) + .getName(); + } + } } catch (Exception e) { handleChangeVariablesError(application2, (Component) owner, e, m); @@ -826,48 +860,81 @@ public class CommunicationManager implements Paintable.RepaintRequestListener { * the HTTP Request. * @param application * the Application to query for window. + * @param assumedWindow + * if the window has been already resolved once, this parameter + * must contain the window. * @return Window mathing the given URI or null if not found. * @throws ServletException * if an exception has occurred that interferes with the * servlet's normal operation. */ private Window getApplicationWindow(HttpServletRequest request, - Application application) throws ServletException { + Application application, Window assumedWindow) + throws ServletException { Window window = null; - // Find the window where the request is handled - String path = request.getPathInfo(); - - // Remove app-runner class-name! - if (applicationServlet.isApplicationRunnerServlet) { - path = path - .substring(1 + applicationServlet.applicationRunnerClassname - .length()); + // If the client knows which window to use, use it if possible + String windowClientRequestedName = request.getParameter("windowName"); + if (assumedWindow != null + && application.getWindows().contains(assumedWindow)) { + windowClientRequestedName = assumedWindow.getName(); + } + if (windowClientRequestedName != null) { + window = application.getWindow(windowClientRequestedName); + if (window != null) { + return window; + } } - // Remove UIDL from the path - path = path.substring("/UIDL".length()); + // If client does not know what window it wants + if (window == null) { - // Main window as the URI is empty - if (path == null || path.length() == 0 || path.equals("/")) { - window = application.getMainWindow(); - } else { - String windowName = null; - if (path.charAt(0) == '/') { - path = path.substring(1); + // Get the path from URL + String path = request.getPathInfo(); + if (applicationServlet.isApplicationRunnerServlet) { + path = path + .substring(1 + applicationServlet.applicationRunnerClassname + .length()); } - final int index = path.indexOf('/'); - if (index < 0) { - windowName = path; - path = ""; - } else { - windowName = path.substring(0, index); - path = path.substring(index + 1); + path = path.substring("/UIDL".length()); + + // If the path is specified, create name from it + if (path != null && path.length() > 0 && !path.equals("/")) { + String windowUrlName = null; + if (path.charAt(0) == '/') { + path = path.substring(1); + } + final int index = path.indexOf('/'); + if (index < 0) { + windowUrlName = path; + path = ""; + } else { + windowUrlName = path.substring(0, index); + path = path.substring(index + 1); + } + + window = application.getWindow(windowUrlName); } - window = application.getWindow(windowName); + } + + // By default, use mainwindow + if (window == null) { + window = application.getMainWindow(); + } + + // If the requested window is already open, resolve conflict + if (currentlyOpenWindowsInClient.contains(window.getName())) { + String newWindowName = window.getName(); + while (currentlyOpenWindowsInClient.contains(newWindowName)) { + newWindowName = window.getName() + "_" + + ((int) (Math.random() * 100000000)); + } + + window = application.getWindow(newWindowName); - // By default, we use main window + // If everything else fails, use main window even in case of + // conflicts if (window == null) { window = application.getMainWindow(); } diff --git a/src/com/itmill/toolkit/tests/tickets/Ticket1970.java b/src/com/itmill/toolkit/tests/tickets/Ticket1970.java index 1ad4a9d1ba..ea40703ddd 100644 --- a/src/com/itmill/toolkit/tests/tickets/Ticket1970.java +++ b/src/com/itmill/toolkit/tests/tickets/Ticket1970.java @@ -1,10 +1,13 @@ package com.itmill.toolkit.tests.tickets; +import java.util.Iterator; + import com.itmill.toolkit.Application; import com.itmill.toolkit.ui.Button; import com.itmill.toolkit.ui.Label; import com.itmill.toolkit.ui.Window; import com.itmill.toolkit.ui.Button.ClickEvent; +import com.itmill.toolkit.ui.Window.CloseEvent; public class Ticket1970 extends Application { @@ -13,15 +16,45 @@ public class Ticket1970 extends Application { } public Window getWindow(String name) { + + // If we already have the requested window, use it Window w = super.getWindow(name); if (w == null) { - w = new Window("Extra window: " + name); - w.setName(name); - addWindow(w); + + // If no window found, create it + w = createExtraWindow(name); } return w; } + private Window createExtraWindow(String name) { + final Window w = new Window("Extra window: " + name); + w.setName(name); + addWindow(w); + w.addComponent(new Label( + "This window has been created on fly for name: " + name)); + w.addComponent(new Button("Show open windows", + new Button.ClickListener() { + + public void buttonClick(ClickEvent event) { + String openWindows = ""; + for (Iterator i = getWindows().iterator(); i.hasNext();) { + Window t = (Window) i.next(); + openWindows += (openWindows.length() > 0 ? "," : "") + + t.getName(); + } + w.showNotification(openWindows); + } + })); + w.addListener(new Window.CloseListener() { + public void windowClose(CloseEvent e) { + removeWindow(w); + } + }); + + return w; + } + private Window createWindow() { final Window w = new Window(); w.addComponent(new Button("Show the name of the application", -- 2.39.5