From 7a90a6a6acc2fbc1cf2a5238b1c6ce719ce53f6d Mon Sep 17 00:00:00 2001 From: Tuukka Lehtonen Date: Wed, 4 Jan 2017 14:38:06 +0200 Subject: [PATCH] Fixed IEditorPart reference (memory) leaks from ResourceEditorSupport ResourceEditorSupport.validationRequest gave IEditorPart as an argument to UnaryRequest which caused the reference to remain in the DB client's caches for very long times. This consumed the JVM heap rather quickly with editors that require a lot of memory and never explicitly free (nullify) it. refs #6936 Change-Id: I46bfd107d57dea0493653afdb378dc089fa508a2 --- .../swt/DefaultExplorerSelectionListener.java | 18 ++- .../ui/swt/GraphExplorerViewBase.java | 13 +- .../ui/workbench/ResourceEditorInput2.java | 7 +- .../ui/workbench/ResourceEditorSupport.java | 112 ++++++++++-------- 4 files changed, 88 insertions(+), 62 deletions(-) diff --git a/bundles/org.simantics.browsing.ui.swt/src/org/simantics/browsing/ui/swt/DefaultExplorerSelectionListener.java b/bundles/org.simantics.browsing.ui.swt/src/org/simantics/browsing/ui/swt/DefaultExplorerSelectionListener.java index 407fbff3b..713b28297 100644 --- a/bundles/org.simantics.browsing.ui.swt/src/org/simantics/browsing/ui/swt/DefaultExplorerSelectionListener.java +++ b/bundles/org.simantics.browsing.ui.swt/src/org/simantics/browsing/ui/swt/DefaultExplorerSelectionListener.java @@ -15,9 +15,10 @@ import org.eclipse.jface.viewers.IPostSelectionProvider; import org.eclipse.jface.viewers.ISelection; import org.eclipse.ui.ISelectionListener; import org.eclipse.ui.IWorkbenchPart; +import org.eclipse.ui.services.IDisposable; import org.simantics.browsing.ui.GraphExplorer; -public class DefaultExplorerSelectionListener implements ISelectionListener { +public class DefaultExplorerSelectionListener implements ISelectionListener, IDisposable { private static final boolean DEBUG = false; @@ -35,6 +36,13 @@ public class DefaultExplorerSelectionListener implements ISelectionListener { this.filter = filter; } + @Override + public void dispose() { + owner = null; + explorer = null; + filter = null; + } + @Override public void selectionChanged(IWorkbenchPart part, ISelection selection) { // Always disregard own selections. @@ -44,6 +52,7 @@ public class DefaultExplorerSelectionListener implements ISelectionListener { if (DEBUG) System.out.println("[" + owner + "] workbench selection changed: part=" + part + ", selection=" + selection); ISelection s = selection; + WorkbenchSelectionFilter filter = this.filter; if (filter != null) s = filter.filterSelection(part, selection); @@ -52,9 +61,12 @@ public class DefaultExplorerSelectionListener implements ISelectionListener { System.out.println("** [" + owner + "] workbench selection changed: part=" + part + ", selection=" + selection); System.out.println(" SETTING NEW SELECTION"); } - if(!explorer.isDisposed()) { + GraphExplorer explorer = this.explorer; + if(explorer != null && !explorer.isDisposed()) { IPostSelectionProvider selectionProvider = (IPostSelectionProvider)explorer.getAdapter(IPostSelectionProvider.class); - selectionProvider.setSelection(selection); + if (selectionProvider != null) { + selectionProvider.setSelection(selection); + } } } else { if (DEBUG) diff --git a/bundles/org.simantics.browsing.ui.swt/src/org/simantics/browsing/ui/swt/GraphExplorerViewBase.java b/bundles/org.simantics.browsing.ui.swt/src/org/simantics/browsing/ui/swt/GraphExplorerViewBase.java index 72307b32f..4b1d8ffef 100644 --- a/bundles/org.simantics.browsing.ui.swt/src/org/simantics/browsing/ui/swt/GraphExplorerViewBase.java +++ b/bundles/org.simantics.browsing.ui.swt/src/org/simantics/browsing/ui/swt/GraphExplorerViewBase.java @@ -27,6 +27,7 @@ import org.eclipse.ui.IViewSite; import org.eclipse.ui.IWorkbenchPart; import org.eclipse.ui.PartInitException; import org.eclipse.ui.part.ViewPart; +import org.eclipse.ui.services.IDisposable; import org.simantics.browsing.ui.Column; import org.simantics.browsing.ui.GraphExplorer; import org.simantics.browsing.ui.NodeContext; @@ -298,6 +299,8 @@ public abstract class GraphExplorerViewBase extends ViewPart { // Remember to remove the installed workbench selection listener if (workbenchSelectionListener != null) { getSite().getWorkbenchWindow().getSelectionService().removePostSelectionListener(workbenchSelectionListener); + if (workbenchSelectionListener instanceof IDisposable) + ((IDisposable) workbenchSelectionListener).dispose(); workbenchSelectionListener = null; getSite().setSelectionProvider(null); @@ -520,16 +523,16 @@ public abstract class GraphExplorerViewBase extends ViewPart { return true; } - @SuppressWarnings("rawtypes") + @SuppressWarnings("unchecked") @Override - public Object getAdapter(Class adapter) { + public T getAdapter(Class adapter) { if (GraphExplorer.class == adapter) - return explorer; + return (T) explorer; else if(ISessionContextProvider.class == adapter) - return getSessionContextProvider(); + return (T) getSessionContextProvider(); else if(IPropertyPage.class == adapter) - return getPropertyPage(); + return (T) getPropertyPage(); return super.getAdapter(adapter); diff --git a/bundles/org.simantics.ui/src/org/simantics/ui/workbench/ResourceEditorInput2.java b/bundles/org.simantics.ui/src/org/simantics/ui/workbench/ResourceEditorInput2.java index e1ec2f521..d40197722 100644 --- a/bundles/org.simantics.ui/src/org/simantics/ui/workbench/ResourceEditorInput2.java +++ b/bundles/org.simantics.ui/src/org/simantics/ui/workbench/ResourceEditorInput2.java @@ -56,7 +56,7 @@ import org.simantics.utils.ui.workbench.StringMemento; public class ResourceEditorInput2 extends PlatformObject implements IResourceEditorInput2, IPersistableElement { private final static boolean DEBUG_EXISTS = false; - private final static boolean DEBUG_UPDATE = false; + private final static boolean DEBUG_UPDATE = true; private static final String NO_NAME = ResourceEditorInput.NO_NAME; @@ -345,9 +345,8 @@ public class ResourceEditorInput2 extends PlatformObject implements IResourceEdi /* (non-Javadoc) * @see org.eclipse.core.runtime.IAdaptable#getAdapter(java.lang.Class) */ - @SuppressWarnings("rawtypes") @Override - public Object getAdapter(Class adapter) { + public T getAdapter(Class adapter) { //System.out.println("[ResourceEditorInput] getAdapter: " + adapter.getName()); return null; } @@ -421,7 +420,7 @@ public class ResourceEditorInput2 extends PlatformObject implements IResourceEdi System.out.println("update(" + this + ")"); try { - assertExists(g); + //assertExists(g); name = g.syncRequest(new TitleRequest(editorID, this)); if (name == null) diff --git a/bundles/org.simantics.ui/src/org/simantics/ui/workbench/ResourceEditorSupport.java b/bundles/org.simantics.ui/src/org/simantics/ui/workbench/ResourceEditorSupport.java index 54a697a72..4075e1e11 100644 --- a/bundles/org.simantics.ui/src/org/simantics/ui/workbench/ResourceEditorSupport.java +++ b/bundles/org.simantics.ui/src/org/simantics/ui/workbench/ResourceEditorSupport.java @@ -21,19 +21,20 @@ import org.simantics.db.ReadGraph; import org.simantics.db.Session; import org.simantics.db.common.procedure.adapter.ListenerAdapter; import org.simantics.db.common.request.ParametrizedRead; -import org.simantics.db.common.request.UnaryRead; +import org.simantics.db.common.request.UniqueRead; import org.simantics.db.event.ChangeEvent; import org.simantics.db.event.ChangeListener; import org.simantics.db.exception.DatabaseException; import org.simantics.db.management.ISessionContext; import org.simantics.db.management.ISessionContextProvider; -import org.simantics.db.request.Read; import org.simantics.db.service.GraphChangeListenerSupport; import org.simantics.ui.SimanticsUI; import org.simantics.utils.datastructures.map.Tuple; import org.simantics.utils.ui.ExceptionUtils; import org.simantics.utils.ui.SWTUtils; import org.simantics.utils.ui.workbench.WorkbenchUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A helper class for easing the attachment of a Simantics database session to @@ -50,6 +51,9 @@ import org.simantics.utils.ui.workbench.WorkbenchUtils; */ public class ResourceEditorSupport implements IAdaptable, ChangeListener { + private static final Logger LOGGER = LoggerFactory.getLogger(ResourceEditorSupport.class); + private static final boolean DEBUG = false; + private IEditorPart editorPart; private ChangeListener editorPartChangeListener; @@ -152,7 +156,7 @@ public class ResourceEditorSupport implements IAdaptable, ChangeListener { return; inputListener = new InputListener(); - getSession().asyncRequest(validationRequest(editorPart), inputListener); + getSession().asyncRequest(new ValidationRequest(), inputListener); } public synchronized void deactivateValidation() { @@ -161,6 +165,7 @@ public class ResourceEditorSupport implements IAdaptable, ChangeListener { if (inputListener == null) return; inputListener.dispose(); + inputListener = null; } public ISessionContext getSessionContext() { @@ -175,19 +180,18 @@ public class ResourceEditorSupport implements IAdaptable, ChangeListener { return session; } - @SuppressWarnings("rawtypes") + @SuppressWarnings("unchecked") @Override - public Object getAdapter(Class adapter) { + public T getAdapter(Class adapter) { if (adapter == ISessionContext.class) - return getSessionContext(); + return (T) getSessionContext(); if (adapter == Session.class) - return getSession(); + return (T) getSession(); return null; } @Override public void graphChanged(ChangeEvent e) throws DatabaseException { - //System.out.println(this + ": graph change: " + e); // Only forward the update to the editor if the input is still valid and // the editor implements ChangeListener if (editorPart instanceof ChangeListener) @@ -225,44 +229,55 @@ public class ResourceEditorSupport implements IAdaptable, ChangeListener { } /** - * @param input - * @return a read request that returns true for valid inputs - * and false for non-existent or invalid inputs. + * A read request that returns an {@link Evaluation} of the current state of + * editorPart. + * + *

+ * This request class is not static but has no parameters that could get + * stuck in the database client caches. UniqueRead does not need arguments + * and without custom hashCode/equals implementations, each instance of this + * request is a different one. This is exactly the behaviour we want in this + * case. */ - private Read validationRequest(IEditorPart editorPart) { - return new UnaryRead(editorPart) { - @Override - public Evaluation perform(ReadGraph graph) throws DatabaseException { - IEditorInput input = parameter.getEditorInput(); - IResourceEditorInput resourceInput = getResourceInput(parameter); - - //System.out.println(ResourceEditorSupport.this + ": checking input " + input); - - boolean exists = true; - boolean valid = true; - if (resourceInput != null) { - exists = resourceInput.exists(graph); - if (exists && inputValidator != null) { - valid = graph.syncRequest(inputValidator.get(resourceInput)); - } - } else { - exists = input.exists(); + private class ValidationRequest extends UniqueRead { + @Override + public Evaluation perform(ReadGraph graph) throws DatabaseException { + IEditorPart part = editorPart; + if (part == null) + return new Evaluation(null, null, InputState.INVALID, "", ""); + + IEditorInput input = part.getEditorInput(); + IResourceEditorInput resourceInput = getResourceInput(part); + + if (DEBUG) + LOGGER.trace("ValidationRequest: checking input " + input); + + boolean exists = true; + boolean valid = true; + if (resourceInput != null) { + exists = resourceInput.exists(graph); + if (exists && inputValidator != null) { + valid = graph.syncRequest(inputValidator.get(resourceInput)); } + } else { + exists = input.exists(); + } - InputState state = InputState.parse(exists, valid); - if (state == InputState.VALID) { - // Make sure any cached data in the editor input is up-to-date. + InputState state = InputState.parse(exists, valid); + if (state == InputState.VALID) { + // Make sure any cached data in the editor input is up-to-date. + if (resourceInput != null) resourceInput.update(graph); - } - - Evaluation eval = new Evaluation(parameter, input, state, input.getName(), input.getToolTipText()); - //System.out.println(ResourceEditorSupport.this + ": validation evaluation: " + eval); - return eval; } - }; + + Evaluation eval = new Evaluation(part, input, state, input.getName(), input.getToolTipText()); + if (DEBUG) + LOGGER.trace("ValidationRequest: evaluation result: " + eval); + return eval; + } } - private class InputListener extends ListenerAdapter { + private static class InputListener extends ListenerAdapter { private boolean disposed = false; @@ -272,11 +287,11 @@ public class ResourceEditorSupport implements IAdaptable, ChangeListener { @Override public void execute(Evaluation evaluation) { - //System.out.println("InputListener: " + evaluation); + if (DEBUG) + LOGGER.trace("InputListener: " + evaluation); switch (evaluation.getInputState()) { case VALID: break; - case INVALID: case NON_EXISTENT: scheduleEditorClose(evaluation.getEditorPart()); @@ -291,18 +306,15 @@ public class ResourceEditorSupport implements IAdaptable, ChangeListener { @Override public boolean isDisposed() { - return disposed || ResourceEditorSupport.this.isDisposed(); + return disposed; } } - private void scheduleEditorClose(final IEditorPart editorPart) { - SWTUtils.asyncExec(editorPart.getSite().getShell(), new Runnable() { - @Override - public void run() { - // Don't have to check isDisposed since closeEditor - // will ignore already closed editor parts. - WorkbenchUtils.closeEditor(editorPart, false); - } + private static void scheduleEditorClose(IEditorPart editorPart) { + SWTUtils.asyncExec(editorPart.getSite().getShell(), () -> { + // Don't have to check isDisposed since closeEditor + // will ignore already closed editor parts. + WorkbenchUtils.closeEditor(editorPart, false); }); } -- 2.43.2