eclipse-sirius / sirius-desktop

Sirius Desktop: desktop-based graphical modelers for dedicated DSLs
https://eclipse.dev/sirius/
Eclipse Public License 2.0
15 stars 11 forks source link

Sync exec deadlock in collaborative mode in oes.common.ui.tools.api.util.EclipseUIUtil.getActiveWindow() #447

Closed GlennPlou closed 2 months ago

GlennPlou commented 2 months ago

A deadlock has been identified in Team for Capella, the stack trace refers to the actual workbench.getDisplay().syncExec(getActiveWorkbenchWindowRunnable); code snippet made in org.eclipse.sirius.common.ui.tools.api.util.EclipseUIUtil.getActiveWindow() . I was not able to reproduce in a Sirius context, but here are the steps to reproduce:

java.lang.IllegalStateException: Call stack for thread Worker-2: Saving In-Flight Entertainment System.team.aird at java.management@17.0.7/sun.management.ThreadImpl.dumpThreads0(Native Method) at java.management@17.0.7/sun.management.ThreadImpl.getThreadInfo(ThreadImpl.java:485) at org.eclipse.ui.internal.UILockListener.reportInterruption(UILockListener.java:214) at org.eclipse.ui.internal.UILockListener.interruptUI(UILockListener.java:180) at org.eclipse.ui.internal.PendingSyncExec.waitUntilExecuted(PendingSyncExec.java:92) at org.eclipse.ui.internal.UISynchronizer.syncExec(UISynchronizer.java:142) at org.eclipse.swt.widgets.Display.syncExec(Display.java:4785) at org.eclipse.sirius.common.ui.tools.api.util.EclipseUIUtil.getActiveWindow(EclipseUIUtil.java:65) at org.eclipse.sirius.common.ui.tools.api.util.EclipseUIUtil.getActivePage(EclipseUIUtil.java:81) at org.eclipse.sirius.common.ui.tools.api.util.EclipseUIUtil.getActiveEditor(EclipseUIUtil.java:94) at org.polarsys.capella.core.sirius.analysis.DiagramServices.getEditPart(DiagramServices.java:2103) at org.polarsys.capella.core.sirius.analysis.DiagramServices.refreshBeginEndLabels(DiagramServices.java:2124) at org.polarsys.capella.core.sirius.analysis.FunctionalChainServices.customizeFunctionalExchangeEdgeLabels(FunctionalChainServices.java:359) at org.polarsys.capella.core.sirius.analysis.FunctionalChainServices.updateFunctionalChainStyles(FunctionalChainServices.java:330) at org.polarsys.capella.core.sirius.analysis.refresh.extension.ComponentArchitectureBlankRefreshExtension.postRefresh(ComponentArchitectureBlankRefreshExtension.java:475) at org.eclipse.sirius.diagram.business.api.refresh.RefreshExtensionService.safePostRefresh(RefreshExtensionService.java:251) at org.eclipse.sirius.diagram.business.api.refresh.RefreshExtensionService.postRefresh(RefreshExtensionService.java:146) at org.eclipse.sirius.diagram.business.internal.sync.DDiagramSynchronizer.refreshOperation(DDiagramSynchronizer.java:398) at org.eclipse.sirius.diagram.business.internal.sync.DDiagramSynchronizer.refresh(DDiagramSynchronizer.java:316) at org.eclipse.sirius.diagram.business.internal.dialect.DiagramDialectServices.refresh(DiagramDialectServices.java:243) at org.eclipse.sirius.business.internal.dialect.DialectManagerImpl.refresh(DialectManagerImpl.java:106) at org.eclipse.sirius.business.api.dialect.command.RefreshRepresentationsCommand.doExecute(RefreshRepresentationsCommand.java:122)



I suggest replacing syncExec with asyncExec, or something similar to https://github.com/eclipse-gmf-runtime/gmf-runtime/commit/2cf3f4ba2aec38f568f2aad848a4543b959734c0. The side effects are not easy to analyze.
mPorhel commented 2 months ago

@GlennPlou please see https://github.com/eclipse-capella/capella/pull/2895

IMO, the issue is in Capella's post refresh extensions and not in this API. Refresh extensions in Capella should not use the helper but retrieve the DialectEditor in a non blocking way

mPorhel commented 2 months ago

For your suggestion: the change from sync to async performed in GMF actions was intended to delay the internal refresh methods of some actions and let them refresh their UI when UI Thread is available, in a non blocking way.

Here the goal is to return the ActiveWindows, and in the Capella post refresh extensions to get the DiagramEditor in order to be able to find its edit part registry. You will see on the PR proposed for Capella that it is possible to tget the dialect editor (active or not) for the currently refreshed diagram.

GlennPlou commented 2 months ago

Issue comes from Capella and their FunctionalChains. We don't have to change anything here in Sirius. See https://github.com/eclipse-capella/capella/pull/2895/