Community
Participate
Working Groups
This is a timing issue and it's rather convoluted, but here it is: BreakpointManager is registered as a resource listener, with the workspace, for events of type IResourceChangeEvent.POST_BUILD. These events are issued during an auto-build job which is ran after every resource change in workspace (whether auto-build is enabled or not). The auto-build job is run with a slight delay (100 ms at most), so that if multiple changes are made before an auto-build job is run, these changes are consolidated into a single delta, when the POST_BUILD event is sent. Now suppose that a debugger UI creates a breakpoint, then after a slight delay, the debugger back end modifies that breakpoint to denote that the breakpoint is planted on the target. The normal order of events in this scenario is that: 1) The breakpoint marker is created and a marker-added event is sent. This marker event is ignored by breakpoint manager, because the manager doesn't send breakpoint-added until the breakpoint is added to the manager. 2) The breakpoint is added to the manager, and the manager sends out a breakpoint-added notification. 3) The breakpoint is modified by the back-end, and a marker-changed resource event is issued. 4) The breakpoint receives the resource event, and issues a corresponding breakpoint-changed event. In the above scenario everything works find, but under some circumstances the following can happen. a) The workspace auto-build is already running as a result of a previous workspace change (an old breakpoint removed for example). 1) The breakpoint marker is created, but the auto-build is delayed 100ms, because the auto-build just ran. Hence the marker-added event is not sent yet. 2) The breakpoint is added to bp manager, and the manager sends out the breakpoint-added event. 3) The debugger back end modifies the breakpoint to mark it as planted, but the marker-changed event is also held up by the auto-build delay. 4) The auto-build is scheduled, and finally the marker event is issued. BUT (and this is the problem) the marker-added and the marker-changed deltas are rolled into one marker-added delta! 5) The manager ignores the marker-added delta, because it already sent out a breakpoint-added event previously. ------------------------------------------------ I attached a hacked PDA example, which marks the BP as planted after planting it, which exhibits this behavior. To reproduce toggle breakpoint to remove it, then immediately toggle a breakpoint again to create it. The newly created breakpoint is never marked as planted. I attached the three files that are modified separately as well. I can see two options to fix this bug: 1. Just listen to IResourceChangeEvent.POST_CHANGED events instead of POST_BUILD. These are sent immediately after the change, so they don't have a chance to be coalesced. The draw-back is that some debuggers may suffer in performance if the make many breakpoint changes in a row instead of performing these changes in a single workbench operation. But the upside is that (besides fixing the bug) the breakpoint updates are actually faster and make the UI look more responsive. (see patch BreakpointManager.java.1) 2. Issue a breakpoint changed event when the marker-added resource change is received. The downside is that a breakpoint-changed evnet would be issued after every breakpoint-add, which is rather ugly. But other than that, events would still be coalesced by auto-build job. (see patch BreakpointManager.java.2)
Created attachment 39448 [details] Hacked PDA example that marks BPs as planted
Created attachment 39449 [details] Hacked PDA example that marks BPs as planted - Target marks BP as planted upon breakpoint-added event.
Created attachment 39450 [details] Hacked PDA example that marks BPs as planted - BP with setPlanted/isPlanted methods
Created attachment 39451 [details] Hacked PDA example that marks BPs as planted - Presentation adds the "(Planted)" to label.
Created attachment 39452 [details] Fix option 1
Created attachment 39453 [details] Fix option 2
This is a great bug. It has a great detailed description of the problem, examples the cause the problem, and suggested fixes with patches. Great bug.
Thanks Darin!
Moving to 3.3
Unfortunately, changing to a POST_CHANGE notification vs. a POST_BUILD notification has the subtle difference that the workspace/resources are not open for change during a POST_CHANGE (and they are open for change during a POST_BUILD). This can causes problems with existing clients that change breakpoints/resources in response to a change in a breakpoint. For example, this caused problems with the java debugger in the past, and still does today: org.eclipse.core.internal.resources.ResourceException: The resource tree is locked for modifications. at org.eclipse.core.internal.resources.WorkManager.checkIn(WorkManager.java:94) at org.eclipse.core.internal.resources.Workspace.prepareOperation(Workspace.java:1684) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1732) at org.eclipse.jdt.internal.debug.ui.JavaDebugOptionsManager.updateBreakpointMessages(JavaDebugOptionsManager.java:765) at org.eclipse.jdt.internal.debug.ui.JavaDebugOptionsManager.breakpointsChanged(JavaDebugOptionsManager.java:779) at org.eclipse.debug.internal.core.BreakpointManager$BreakpointsNotifier.run(BreakpointManager.java:860) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:37) at org.eclipse.debug.internal.core.BreakpointManager$BreakpointsNotifier.notify(BreakpointManager.java:879) at org.eclipse.debug.internal.core.BreakpointManager.fireUpdate(BreakpointManager.java:733) at org.eclipse.debug.internal.core.BreakpointManager.access$1(BreakpointManager.java:720) at org.eclipse.debug.internal.core.BreakpointManager$BreakpointManagerVisitor.update(BreakpointManager.java:600) at org.eclipse.debug.internal.core.BreakpointManager.resourceChanged(BreakpointManager.java:536) at org.eclipse.core.internal.events.NotificationManager$2.run(NotificationManager.java:282) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:37) at org.eclipse.core.internal.events.NotificationManager.notify(NotificationManager.java:276) at org.eclipse.core.internal.events.NotificationManager.broadcastChanges(NotificationManager.java:148) at org.eclipse.core.internal.resources.Workspace.broadcastPostChange(Workspace.java:256) at org.eclipse.core.internal.resources.Workspace.endOperation(Workspace.java:958) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1746) at org.eclipse.debug.core.model.Breakpoint.setAttribute(Breakpoint.java:190) at org.eclipse.jdt.internal.debug.core.breakpoints.JavaBreakpoint.decrementInstallCount(JavaBreakpoint.java:755) at org.eclipse.jdt.internal.debug.core.breakpoints.JavaBreakpoint.deregisterRequest(JavaBreakpoint.java:244) at org.eclipse.jdt.internal.debug.core.breakpoints.JavaBreakpoint.removeRequests(JavaBreakpoint.java:673) at org.eclipse.jdt.internal.debug.core.breakpoints.JavaBreakpoint.removeFromTarget(JavaBreakpoint.java:616) at org.eclipse.jdt.internal.debug.core.breakpoints.JavaLineBreakpoint.removeFromTarget(JavaLineBreakpoint.java:177) at org.eclipse.jdt.internal.debug.core.model.JDIDebugTarget.removeAllBreakpoints(JDIDebugTarget.java:1379) at org.eclipse.jdt.internal.debug.core.model.JDIDebugTarget.cleanup(JDIDebugTarget.java:1337) at org.eclipse.jdt.internal.debug.core.model.JDIDebugTarget.terminated(JDIDebugTarget.java:1302) at org.eclipse.jdt.internal.debug.core.model.JDIDebugTarget.handleVMDisconnect(JDIDebugTarget.java:903) at org.eclipse.jdt.internal.debug.core.EventDispatcher.dispatch(EventDispatcher.java:154) at org.eclipse.jdt.internal.debug.core.EventDispatcher.run(EventDispatcher.java:232) at java.lang.Thread.run(Thread.java:534) So, although I like the simplicity of fix 1, we can't use it. I also don't like the idea of firing needless change notifications (fix 2). Perhaps we can match up add/register notifications and only send the add notification once a breakpoint is registered and its marker has been added.
This is indeed unfortunate :-( I suppose asking the clients to perform their modifications inside a job and using an appropriate scheduling rule is a little too much to ask. I'm actually really surprised that Workspace.run() doesn't block until the resource tree is unlocked... even if in this case it would have deadlocked the resource system. As an alternative, could the breakpoint manager dispatch the breakpointChanged() events in a separate job? I think the place to try it would be when BreakpointManagerVisitor.update() is called. In fact if this job would call the listeners inside of a Workspace.run() operation, it would guarantee that if the listeners modified the resource tree, they would not run into a resource tree blocked exception. On the downside this is a pretty radical approach, and it would require a lot of testing. One place where this strategy might run into trouble, is that BreakpointManagerVisitor instance fgVisitor would have to be re-entrant, because it could be called with a marker changed notification, while it's still processing a previous one. The strategy of avoiding sending the added event until the breakpoint is registered would work too, it's probably the safest change. On the downside it would still have the occasional delay that is caused by the events being sent as part of the auto-build job.
Fixed. The breakpoint manager now registeres a POST_CHANGE listener and a POST_BUILD listener. This way, it can keep track of which breakpoints have changed before we received a "POST_BUILD add" notification, and fire a corresponding change notification for breakpoints that have changed since we notified listeners of the "add". We still only notify listeners during POST_BUILD.
Please verify, Kevin.
verified
