Community
Participate
Working Groups
Build ID: 3.4.0.v20080610 I am a performance analyst of an adopting product. I have been chasing down an intermittent issue that I have tracked to the org.eclipse.core.internal.resources.Workspace org.eclipse.core.internal.events.NotificationManager classes. Here is the flow that results in the problem: (1) A resource change results in a scheduling of NotificationManager.NotifyJob (2) An IWorkspaceRunnable is entered before the necessary resource change notifications are reported (3) The IWorkspaceRunnable creates batches of resources that must grouped together during resource change notifications (or else resource change listeners will fail)--hence the IWorkspace.AVOID_UPDATE option is given to the Workspace.run method (4) During execution of the IWorkspaceRunnable, the NotifyJob runs, setting NotificationManager.notificationRequested to 'true' (5) On the next resource creation after the NotifyJob runs, Workspace.endOperation is called (after the resource creation, not the one after the whole IWorkspaceRunnable) and even though the current thread is in the NotificationManager.avoidNotify set, the method NotificationManager.shouldNotify() only checks whether a current notification is in progress or whether 'notificationRequested' is true (which it is in this case). So, in Workspace.endOperation, this code: if (!(notificationManager.shouldNotify() || depthOne)) { notificationManager.requestNotify(); return; } skips over the NotificationManager.requestNotify() (which DOES check the avoidNotify set) and return statements, and proceeds to do the notifications. This results in notifications being made for an incomplete set of resource creations in my IWorkspaceRunnable, which are picked up by a resource change listener that expects all the resources to be there.... For reference, here is an example stack trace showing the problem: at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathUpdater.resourceChanged at org.eclipse.core.internal.events.NotificationManager$2.run at org.eclipse.core.runtime.SafeRunner.run at org.eclipse.core.internal.events.NotificationManager.notify at org.eclipse.core.internal.events.NotificationManager.broadcastChanges at org.eclipse.core.internal.resources.Workspace.broadcastPostChange at org.eclipse.core.internal.resources.Workspace.endOperation at org.eclipse.core.internal.resources.File.create at org.eclipse.core.internal.resources.File.create at <product class 1>.createFile at <product class 1>.run at <product class 2>.run at org.eclipse.core.internal.resources.Workspace.run at <product class 3>.run at org.eclipse.core.internal.resources.Workspace.run It appears that there are 3 options to fix this problem: (a) Check for a scheduled NotificationManager.NotifyJob before running the IWorkspaceRunnable and perform pending notifications. (b) Cancel a scheduled NotificationManager.NotifyJob before running the IWorkspaceRunnable and allow the pending changes to be reported at the end of the IWorkspaceRunnable. (c) Allow notifications during the IWorkspaceRunnable, but only for those changes that occurred before the IWorkspaceRunnable. I think that (a) or (b) would be more feasible than (c). I will try to create a test case that clearly shows the problem (are there any existing test cases that would be a good starting point?), but I think I've given enough detail that the problem should be understood as is.
You have described the situation well, but this is expected behaviour. Imagine the IWorkspaceRunnable is a long running operation that may take minutes to complete. Now during that time the user adds a breakpoint to a Java class. If we can't notify of the workspace change until the end of the long-running operation, the effect will be that they double-click to add the breakpoint, and several minutes later the breakpoint annotation appears. So although AVOID_UPDATE is taken as a hint that intermediate changes not be notified, resource change events must occur anyway if changes occur in another thread.
And if changes only occur on the same thread? Does the story change?
The notifications that I think should be avoided are those made in the IWorkspaceRunnable requesting AVOID_UPDATE (same thread only). The problem as reported occurs during a build, meaning that workspace changes are only allowed in the build thread (which is the thread running this IWorkspaceRunnable).
I see, in this case there was an unrelated change immediately before the IWorkspaceRunnable in the same thread. It is possible we could avoid the notification in this case using your solution (a). Still you can never be absolutely guaranteed that intermediate resource change events don't occur because you can't control what is happening in other threads.
I assume from your comments that it is not feasible to segregate resource changes by thread, thereby allowing changes made by an IWorkspaceRunnable to be deferred, while changes made on other threads are not deferred?
From the javadoc for IWorkspace.run(..): "The AVOID_UPDATE flag controls whether periodic resource change notifications should occur during the scope of this call. If this flag is specified, and no other threads modify the workspace concurrently, then all resource change notifications will be deferred until the end of this call. If this flag is not specified, the platform may decide to broadcast periodic resource change notifications during the scope of this call." According to this description, this is definitely a bug, since no other threads were the source of the resource change. If this bug is not fixed, then the javadoc should at least be changed to clearly describe that the AVOID_UPDATE flag is only a hint and guarantees nothing (even on the same thread).
re comment #5: correct, this would result in listeners receiving resource change events in a different order from the order in which changes occurred, which can be problematic for listeners. Listeners often (reasonably) assume that the resource delta accurately describes the state of the world at the moment the event is broadcast. re comment #6: yes, I agree this is a bug according to that spec. I was just suggesting this is quite minor since there are other ways intermediate notifications could be triggered.
Ok. We are in agreement. In any case, my problem listener and the code it uses (which happens to be Eclipse WST code) needs to be able to handle this properly. I am OK with setting the severity of this bug to minor.
I opened https://bugs.eclipse.org/bugs/show_bug.cgi?id=250004 to address working around this issue in the affected WTP code.
*** Bug 306214 has been marked as a duplicate of this bug. ***
Bug 306214 describes a similar case where listeners are notified if another runnable runs even not modifying the workspace. In case we decide to clear spec (stating AVOID_UPDATE is only a hint) we're good. Otherwise this another way for unexpected notifications has to be considered.
Some additional comment to bug 306214: (In reply to bug 306214 comment #17) > Even if the other runnable doesn't modify the workspace relaying on the flag is > error prone. You can't make assumption on other threads. In your case deadlock > would occur sooner or later even if we follow the spec exactly. In our case we first do some analysis if acquiring the lock in the resource changed listener will be really necessary. Concrete, we don’t acquire the lock on marker changes. So hopefully our code wouldn’t be error prone. Or are there any other cases in that clients are allowed to modify the workspace while my runnable with the workspace scheduling rule is performing? (In reply to bug 306214 comment #17) > This is not a critical bug then, lowering severity to normal. I agree. We found a workaround that works for us. See attached unit test on bug 306214. (In reply to bug 306214 comment #17) > It's not an option as it breaks existing clients and makes the whole workspace > less responsive and less robust, see bug 249951 comment 1. I’m not convinced that the impact will be that much. But actually, there would be an impact. If you decide not to change the current behavior, pleas spent some time to adjust the spec. Maybe, some additional sentences in the "Tips and tricks for listener implementers: Thread safety" section of John Arthornes article "How You’ve Changed!" would be an good idea.
(In reply to comment #12) > In our case we first do some analysis if acquiring the lock in the resource > changed listener will be really necessary. Concrete, we don’t acquire the lock > on marker changes. So hopefully our code wouldn’t be error prone. Or are there > any other cases in that clients are allowed to modify the workspace while my > runnable with the workspace scheduling rule is performing? A thread can modify markers while another one executes within a rule. This is because marker modification has a null default rule (ResourceRuleFactory, Rules). Modifying a marker results in a delta being fired. Spec for IWorkspace.run() says that AVOID_UPDATE don't apply if other thread is modifying the workspace (e.g. changing a marker). This shows the code is error prone. Anyway, default rule for an operation can be overridden. For example for resource creation the default rule is parent but it can be overriden to be null. Thus holding a lock does not guarantee that workspace can't be modified by other thread resulting in a delta.
(In reply to comment #13) > A thread can modify markers while another one executes within a rule. This is > because marker modification has a null default rule (ResourceRuleFactory, > Rules). Modifying a marker results in a delta being fired. Spec for > IWorkspace.run() says that AVOID_UPDATE don't apply if other thread is > modifying the workspace (e.g. changing a marker). This shows the code is error > prone. What Bernd is saying is that a ResourceChanger listener obviously will not acquire the data structure lock on every kind of resource change. Marker updates (usually?) don't require changes in the client data model thus the listener code should/will NOT acquire the data structure lock on marker changes. > Anyway, default rule for an operation can be overridden. For example for > resource creation the default rule is parent but it can be overriden to be > null. Thus holding a lock does not guarantee that workspace can't be modified > by other thread resulting in a delta. While this is true are you saying that this is something that should be anticipated? What's the point in scheduling rules then when the workspace can be changed even when they are held? I was under the asumption that while this is possible no one should do it.
(In reply to comment #14) > What Bernd is saying is that a ResourceChanger listener obviously will not > acquire the data structure lock on every kind of resource change. Marker > updates (usually?) don't require changes in the client data model thus the > listener code should/will NOT acquire the data structure lock on marker > changes. I meant also events recordered before marker modification. Which are send in such case not to corrupt event order.
(In reply to comment #14) > While this is true are you saying that this is something that should be > anticipated? I'm only saying given the design of Resources it's hard to assume when the listeners are being notified. Thus a special care should be taken while acquiring locks in reaction to a resource change.
(In reply to comment #15) > I meant also events recordered before marker modification. Which are send in > such case not to corrupt event order. Oh dear, I see. Now I understand why it is useless to change the behaviour according to the sepc. But these marker changes with null rules means that the resource API won't guarantee any atomic operation within the workspace. Or in other words, the resource API doesn't provide any thread safe mechanism to modify files. Even if the doc of the IWorkspace.run(IWorkspaceRunnable, IProgressMonitor) says: "Runs the given action as an atomic workspace operation". (Please tell me that I'm wrong) What's the point in resource scheduling rules if not to enable atomic resource operations? What's about e4? Are there any further concepts to establish real atomic resource operations, e.g. read/write rules?
(In reply to comment #17) > Or in other words, the resource API doesn't provide any thread safe mechanism > to modify files. To achieve it you could provide a custom rule factory that return non null rules even for marker changes. The current defaults are intentional, see bug 42080.
Created attachment 165601 [details] example back-trace I'm bumping this to major as I can hit this repeatably using a new CDT feature. The result is things seriously break - the deadlock detector releases the WorkspaceLock during a notification to another thread which can then do a notification on operation end giving: org.eclipse.core.runtime.AssertionFailedException: assertion failed: The workspace tree is already locked Deadlock detected. All locks owned by thread Worker-6 will be suspended. Thread Worker-6 has locks: OrderedLock (0) and is waiting for lock OrderedLock (80) Thread Worker-3 has locks: R/, OrderedLock (80) and is waiting for lock OrderedLock (0) As I see it, I'm acquiring locks in the right order. If the API user is modifying resources, we ensure that resource scheduling rules are acquired before the data structure lock (80). However as there's no way of explicitly acquiring the workspaceLock, it's impossible to avoid this deadlock with the notification thread... There are two solutions (I can see): - Absolutely forbid accessing the CDT project model during resource change notification - this is near on impossible - Always perform the serialization asynchronously. This is also not ideal...
(In reply to comment #19) > As I see it, I'm acquiring locks in the right order. If the API user is > modifying resources, we ensure that resource scheduling rules are acquired > before the data structure lock (80). However as there's no way of explicitly > acquiring the workspaceLock, it's impossible to avoid this deadlock with the > notification thread... This is exactly the same kind of problem and the same kind of deadlock we have (thread dump on bug 306214). > There are two solutions (I can see): Maybe we have a third one: - To achive "atomic" workspace operations we are using the workaround to cancel the NotificationManager$NotifyJob each time he gets scheduled while one of our workspace runnables is performing (see unit test on 306214). Also not ideal...
I attempted to work around this bug by deferring the resource write to a background thread. This seems to work OK except it trips a bunch of tests that expect to: - set the CDT project description - close the project - open the project and check the changes have persisted. AFAICS I can't stop the project close (and the workspace is closed for modification during PRE_CLOSE). So it's not possible to ensure the serialization has occurred before the project close. The other thing I could do is bypass the IResource API entirely. Write the project file in-line using EFS. Then schedule a background job to refresh the project file. I then lose the ability to have local history, but this may be the least intrusive workaround. Are there any better ideas?
(In reply to comment #21) > Are there any better ideas? See comment above. The workaround realy works, give it a chance. Your CProjectDescriptionManager.runAtomic(IWorkspaceRunnable, ISchedulingRule, IProgressMonitor) method could wrap the given runnable with a internal runnable that registeres a job change listener that cancels all newly scheduled NotifyJobs (NotifyJobCanceler, see snipped below). Then the internal runnable runs your outer runnable and finally deregisters the job listener. The occurred resource events arn't lost, they will be sent on leaving the internal runnable. For detailed information take a look at the unit test attached on bug 306214. private static final class NotifyJobCanceler extends JobChangeAdapter { @Override public void aboutToRun(IJobChangeEvent event) { final Job job = event.getJob(); if ("org.eclipse.core.internal.events.NotificationManager$NotifyJob".equals(job.getClass().getName())) { job.cancel(); } } }
(In reply to comment #22) > (In reply to comment #21) > > Are there any better ideas? > > See comment above. The workaround realy works, give it a chance. Thanks Bernd! I thought it might not work as there're other cases where broadcastPostChange() is called. However it looks like you're right: the only other two places are #aboutToBuild (which is called with WR rule held) and Workspace#checkpoint (which isn't called?). It looks like this will indeed do the trick, thanks! :)
Another of my pains came around to this problem. We are laying out JEE module files using JET and IFile.create/setContents and sometimes the NotificationManager kicks in at exactly the wrong time and causes WST to load incomplete models bringing up an Inconsistent Files pop-up incorrectly.
*** Bug 341360 has been marked as a duplicate of this bug. ***
(In reply to comment #17) > What's the point in resource scheduling rules if not to enable atomic resource > operations? In the current workspace semantics, scheduling rules prevent you from concurrently modifying resources i.e. setContents, delete, move, close, touch using the IResource API. For modifying markers and other metadata no resource scheduling rule is required (for the reason outline in comment 1). The main issue I've had with this is deadlock: comment 19, and bug 306214 closed as a duplicate of this. As clients aren't aware of the WS lock, holding it while calling into client code can easily deadlock if the client ever holds the lock while calling into core.resources. The Notification manager already prevents multiple concurrent notifications, so fixing this shouldn't be too hard. I think this is the main thing we can fix here. Deferring notification indefinitely isn't going to happen, so changing the summary.
James I'm not sure what you are suggesting here. Are you suggesting we don't lock at all, or that we use a different form of locking?
(In reply to comment #27) > James I'm not sure what you are suggesting here. Are you suggesting we don't > lock at all, or that we use a different form of locking? Locking still happens as before. The main change would be in: broadcastChanges(ElementTree lastState, ResourceChangeEvent event, boolean lockTree); We could: 1) Make the tree lastState.immutable() 2) beingUnprotected() before calling notify(getListeners(), event, lockTree); During the notification, resource changes would be permitted concurrently in other threads (using a new tree generation). Post change is calculated from the last saved tree state, so this should continue to work correctly. Also the isNotifying guard will prevent more than one notification at once (though looking at the logic in #requestNotify, we'll need a test to ensure we don't miss notifications indefinitely). Can you see any reason why this wouldn't work?
(In reply to comment #28) > During the notification, resource changes would be permitted concurrently in > other threads (using a new tree generation). I really wish I had documented this better, but I tried this approach when we first moved auto-build out of endOperation (see Workspace.java revisions 1.99.2.4 through 1.99.2.11). The main reason it failed is that listeners were relying heavily on the promise made in the IResourceChangeEvent contract ("The workspace is closed for change during notification of these events."). For example if they received an ADDED delta, they would assume the ADDED file existed at the time the listener was called. If they received a MOVED_TO delta, they would make the assumption the source didn't exist and the destination did exist. Generally, they make the assumption that the resource delta accurately describes the world at the time the listener is running. If the workspace can be changing *while* the listener is running, then the delta just tells them what the world was like at some point in the recent past. I fully agree the title of this bug is a true statement: Holding the WS lock during resource delta notification can lead to deadlock. This unfortunately follows directly from our API promise that the workspace is locked during resource change events. The advice we have always given to resolve this is, "don't acquire locks in your resource listener that could be held while modifying the workspace". If a resource change listener is able to tolerate running asynchronously, then it can simply schedule a job to perform its updates, and don't do any real work within the body of the listener.
Thanks for the comment John. (In reply to comment #29) > The main reason it failed is that listeners were > relying heavily on the promise made in the IResourceChangeEvent contract ("The > workspace is closed for change during notification of these events."). For > example if they received an ADDED delta, they would assume the ADDED file > existed at the time the listener was called. If they received a MOVED_TO delta, > they would make the assumption the source didn't exist and the destination did > exist. Generally, they make the assumption that the resource delta accurately > describes the world at the time the listener is running. If the workspace can > be changing *while* the listener is running, then the delta just tells them > what the world was like at some point in the recent past. There are a few reasons I think this isn't as bad as it sounds: - Callers of the Resources API *must* already handle CoreException for external changes occurring concurrently under Eclipse's feet: e.g. external build tools running outside of eclipse. - PRE_BUILD + POST_BUILD listeners can modify resources which will affect future listeners examining #getDelta(). - Resource delta can already fire halfway through a move: IResource#move runs unprotected, so deltas can fire during the Workspace#copyTree > I fully agree the title of this bug is a true statement: Holding the WS lock > during resource delta notification can lead to deadlock. This unfortunately > follows directly from our API promise that the workspace is locked during > resource change events. The advice we have always given to resolve this is, > "don't acquire locks in your resource listener that could be held while > modifying the workspace". If a resource change listener is able to tolerate > running asynchronously, then it can simply schedule a job to perform its > updates, and don't do any real work within the body of the listener. It's worse than this though. It makes it impossible to update a model which has some resource based dependency. I would argue that any useful model, and certainly anything which wants to modify resources, _already_ needs to schedule work in an asynchronous job. Clients therefore expect and have to handle concurrent WS modification with asynchronous notification. Scheduling rules provide a good mutex mechanism for client modification. However resource callback notification is essentially asynchronous -- clients are eventually notified of all changes with an arbitrary delay. There appear to be very many dupes of this bug. Given the care taken to ensure we don't hold locks while calling client code everywhere else, this bit of API stands out sorely. It makes resource deltas very hard to use and, at the end of the day, doesn't actually protect the client from any concurrent modification already possible today. Making the proposed change here doesn't break existing tests. (Undoubtedly some more work would be needed to ensure that there are no races or delayed events.) From my POV as clients have no way of obtaining this lock, it will continue to be a deadlock trap with any non-trivial listener code.
(In reply to comment #30) > It's worse than this though. It makes it impossible to update a model which > has some resource based dependency. Bug 310007 is a good example of why this can be impossible. In CDT we have API that mirrors core.resources: #getProjectDescription() #setProjectDescription() Our models are large and lazily loaded. listeners are notified of various events: LOAD, APPLYING, etc. We don't want to allow two concurrent loads. As a result #getPD() uses a lock to prevent concurrently load / save. (Much like the natural synchronization in ElementTree.) #getPD _never_ calls into the workspace. #setPD also uses the lock, and calls into the workspace to serialize the model. The problem is that clients expect to be able to access the project description during a resource change notification. This can infrequently deadlock with another client saving the PD. With 10s of integrations, we can't prevent clients from calling #get from a res change listener. The alternative would be to stop the #set calling into core.resources. This too is hard - up till now this API has been synchronous: when the #set completes successfully, your data is saved to disk. We could send this to a job, but this leads to the same issues seen with delaying the Platform's #setDescription (comment 21). The hack we've got at the moment is Bernd's comment 22: we cancel the notification job for the duration of the #set. I think our integration is similar to other integrations on top the platform. Without re-architecting it to be lock free, this bug makes it very difficult for clients to call any complex API as the result of a resource change event.
This also raises potentially large performance concern: if one of the user's plugins installs a heavyweight resource change handler, then all resource changing actions in the IDE will be rate limited by that handler's performance. For example: See backtrace in bug 323839 comment 34 https://bugs.eclipse.org/bugs/attachment.cgi?id=204057 (+ bug 358898) egit has a really expensive resource change handler. ctrl-S then locks up the UI for minutes. ctrl-S could report progress (bug 336612), but I'd still be sat there for a minute waiting for the editor to serialize the document. By making resource change synchronous, we bound workspace throughput to the throughput of the worst performing resource change handler.
*** Bug 342219 has been marked as a duplicate of this bug. ***
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
This bug was marked as stalebug a while ago. Marking as worksforme. If this report is still relevant for the current release, please reopen and remove the stalebug whiteboard tag.
This bug has been marked as stalebug a while ago without any further interaction. If this report is still relevant for the current release, please reopen and remove the stalebug whiteboard flag.
