eclipse.platform icon indicating copy to clipboard operation
eclipse.platform copied to clipboard

Use workspace root modification rule when removing all breakpoints

Open trancexpress opened this issue 2 months ago • 5 comments

When removing breakpoints, its possible that another job is accessing the breakpoint marker. This can result in an exception, since removing a breakpoint will also remove the marker.

This change moves breakpoint removal code in a job that holds a workspace root modification rule, if the current thread does not hold a matching rule.

Fixes: #2217

trancexpress avatar Oct 28 '25 12:10 trancexpress

This revives the now gone: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/165753

trancexpress avatar Oct 28 '25 12:10 trancexpress

Test Results

 1 947 files  ±0   1 947 suites  ±0   1h 40m 12s ⏱️ -26s  4 721 tests ±0   4 697 ✅ ±0   24 💤 ±0  0 ❌ ±0  14 163 runs  ±0  13 996 ✅ ±0  167 💤 ±0  0 ❌ ±0 

Results for commit 00994ba0. ± Comparison against base commit 018c6675.

github-actions[bot] avatar Oct 28 '25 13:10 github-actions[bot]

Would just passing the rule MultiRule.combine(update.stream().map(getWorkspace()::markerRule).toArray(ISchedulingRule[]::new)) prevent from comcurrent access without blocking too much?

mickaelistria avatar Oct 30 '25 08:10 mickaelistria

Would just passing the rule MultiRule.combine(update.stream().map(getWorkspace()::markerRule).toArray(ISchedulingRule[]::new)) prevent from comcurrent access without blocking too much?

I'll check when I can, I'd have more confidence in the change that way.

If you mean org.eclipse.core.resources.IResourceRuleFactory.markerRule(IResource), it returns null always?

trancexpress avatar Oct 30 '25 08:10 trancexpress

The MultiRule suggestion works, I wonder if we should use it also in JavaDebugOptionsManager... https://github.com/eclipse-jdt/eclipse.jdt.debug/commit/897972b60a60c5bd9d0b8138d5be9b07addfcaf5

When testing the change, I noticed its not enough to prevent the logged error. This code is removing the info:

"Worker-18: Remove Breakpoints" #131 [77058] prio=5 os_prio=0 cpu=418.64ms elapsed=1291.43s tid=0x00007f5d68000de0 nid=77058 at breakpoint [0x00007f5da91ee000]
   java.lang.Thread.State: RUNNABLE
        at org.eclipse.core.internal.resources.MarkerManager.removeMarker(MarkerManager.java:513)
        at org.eclipse.core.internal.resources.Marker.delete(Marker.java:84)
        at org.eclipse.ui.ide.undo.AbstractMarkersOperation.deleteMarkers(AbstractMarkersOperation.java:117)
        at org.eclipse.ui.ide.undo.DeleteMarkersOperation.doExecute(DeleteMarkersOperation.java:61)
        at org.eclipse.debug.internal.ui.views.breakpoints.DeleteBreakpointMarkersOperation.doExecute(DeleteBreakpointMarkersOperation.java:48)
        at org.eclipse.ui.ide.undo.AbstractWorkspaceOperation.lambda$0(AbstractWorkspaceOperation.java:199)
        at org.eclipse.ui.ide.undo.AbstractWorkspaceOperation$$Lambda/0x00007f5e94bdc8c8.run(Unknown Source)
        at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2505)
        at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2533)
        at org.eclipse.ui.ide.undo.AbstractWorkspaceOperation.execute(AbstractWorkspaceOperation.java:199)
        at org.eclipse.core.commands.operations.DefaultOperationHistory.execute(DefaultOperationHistory.java:488)
        at org.eclipse.debug.ui.DebugUITools.deleteBreakpoints(DebugUITools.java:353)
        at org.eclipse.debug.internal.ui.actions.breakpoints.RemoveAllBreakpointsAction$1.run(RemoveAllBreakpointsAction.java:126)
        at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

So apparently there is more to change...

trancexpress avatar Oct 30 '25 15:10 trancexpress