Community
Participate
Working Groups
This is a proposal to improve the perceived sync state problems by registering out-of-sync resources with the RefreshManager for asynchronous update. Note that this needn't change the semantics of IResource operations, nor should it impose much additional overhead. As part of Resource changing operations (and some Resource reading operations e.g. IFile#getContents()), sync state is checked using Resource#isSynchronized(). If this returns false, and the user hasn't specified force, an exception is thrown and the operation will fail. If, on discovery of resource out-of-sync, the resource was added to the RefreshManager queue for asynchronous refresh, then I believe the user experience would improve substantially. Importantly: - the api would behave the same - if force isn't specified, an exception would still be thrown by the respective API calls. However it would improve a number of use-cases: - Editors, for out-of-sync files, would not require manual refresh of the file. - "File Search" needn't report "Resource out of sync errors", and out of sync files are automatically brought back into sync. Most users expect the workspace to be in sync most of the time. When it's not they view it as an Eclipse problem rather than their problem. Trying to explain to someone why they must manually refresh when the IDE already knows the file is out of sync can be tough...
This sounds very interesting! (In reply to comment #0) > - Editors, for out-of-sync files, would not require manual refresh > - "File Search" needn't report "Resource out of sync errors", > and out of sync files are automatically brought back into sync. I don't understand how this would work in practice. Assume that workspace thinks that file "foo" is there. Now, a search (or editor) finds that it is out-of-sync because foo has been deleted on disk. RefreshManager is triggered to bring workspace back into sync, but that happens asynchronously (who knows when). What would the search, or the editor do in this case? Today it throws the error because resource is out of sync. With your new proposal, how would the need for manual refresh / need for reporting in search be avoided? > Most users expect the workspace to be in sync most of the time. When it's not > they view it as an Eclipse problem rather than their problem. I whole-heartedly agree on this one. The normal behavior should be to auto-refresh whenever it looks like a refresh is needed. There might be very few cases where I deliberately work without automatic refresh in order to improve performance, but I'd expect that this mode of operation is for very advanced users only.
(In reply to comment #1) > I don't understand how this would work in practice. Assume that workspace > thinks that file "foo" is there. Now, a search (or editor) finds that it is > out-of-sync because foo has been deleted on disk. RefreshManager is triggered > to bring workspace back into sync, Editors actually behave reasonably well under these circumstances -- there seems to be periodic checking of sync state when focus changes. - If you open an editor, delete the file underneath it, and select it, you get a dialog "File has been deleted or is not accessible. Do you want to Save?" [Note when you try to save you get: File already exists, do you want to replace even though the file no longer exists...] - If you delete the file, then try to open it, you get "Resource is out of sync with fs, Press F5 to Refresh. AFAICS if the file's deleted externally to Eclipse there's no way to recover it within the IDE? > but that happens asynchronously (who knows > when). Indeed. The aim is to reduce the lag in sync without affecting the existing semantics of the Resources API. > What would the search, or the editor do in this case? Today it throws the error > because resource is out of sync. With your new proposal, how would the need for > manual refresh / need for reporting in search be avoided? In the Text search case the grep itself doesn't much care about file sync state. It could call IFile#getContents(true) and populate the search results view as before, not reporting an error on out-of-sync files. The resources plugin would queue these files for refresh. This would be equivalent to the existing asynchronous IRefreshMonitor(s) (including the built-in polling monitor) which notify RefreshManager that certain resources need to be refreshed after external change. As soon as the sync state is up to date, the out-of-sync editor is replaced by the actual file contents as the user expects and happens currently. > improve performance, but I'd expect that this mode of operation is for very > advanced users only. That's an interesting point: there's currently only one preference for auto-refresh. This global toggle predicates most the automatic refresh actions (with some exception - crash recovery, unknown children population). It seems like there would need to be two options: auto-refresh & polling-refresh for platforms without a refresh monitor. The former is intended for lightweight monitors & as a standard default, the latter would disable expensive polling on other platforms.
*** Bug 305158 has been marked as a duplicate of this bug. ***
Created attachment 188377 [details] prototype patch Example of how this might be fixed in FileSystemResourceManager. The patch simply registers discovered out-of-sync resources with the refresh manager for asyncRefresh. - #read (called by #getContents) throws CoreException on out-of-sync. Playing with it shows editor updates with sub-second speed after discovered out-of-sync - #isSynchronized discovered whether files are in sync. Such discovery is potentially expensive (in I/O terms) and as out-of-sync resources are useless to IResource consumers also schedules them for refresh. This will definitely break tests which try to provoke out of sync.... In both cases semantics of the API are unchanged. From API hooks' POV out-of-sync resources come back into sync asynchronously via refresh monitor, user refresh, or other api call with 'force' set. The proposed approach simply ensure this happens in a more timely manner. There are many other places which throw IResourceStatus.OUT_OF_SYNC_LOCAL. But the patch shows what can be done and how this increases usability and reduces the burden on integrators to refreshLocal all the time...
Created attachment 189175 [details] fix + tests 1 Fix for the issue: - FileSystemResourceManager: read / write / isSynchronized schedule discovered out-of-sync IResource for refresh with refresh manager - IsSynchronizedVisitor: keeps track of resource discovered out-of-sync, handle resource gender changes Tests assert that when core.resources discovered an out-of-sync resource, sync is restored when Job FAMILY_AUTO_REFRESH is done: - #getContents() out-of-sync resource - #getContents() on deleted resource - #getContents(true) - #isSynchronized - out-of-sync - #isSynchronized - gender change (File -> Folder)
Szymon this fixes the out-of-sync issues from the p.o.v. of core.resources. In most instances the resource will be refreshing within a split second (job scheduled after 200ms) so users will be spared the "Press F5 to refresh message". Discovering a resource is out-of-sync involves I/O and is expensive. Having made this discovery it makes sense that core.resources schedules the resource for update saving user intervention -- there's nothing to be gained by not doing this (other than a frustrated user in the future...). It does seem to provoke an editor bug though: A clean editor, modified externally, has it's content changed before the user has acknowledged the "The file '...' has changed dialog'. As we haven't changed the semantics of core.resources API calls, this would occur with normal auto-refresh on too. I'll investigate this. Dirty editors, and opening editors on out-of-sync / removed files works as expected.
> It does seem to provoke an editor bug though: > A clean editor, modified externally, has it's content changed before the user > has acknowledged the "The file '...' has changed dialog'. As we haven't > changed the semantics of core.resources API calls, this would occur with normal > auto-refresh on too. I'll investigate this. No need. This is as designed.
(In reply to comment #7) > No need. This is as designed. Agreed it doesn't matter if a non-dirty editor has its content replaced. However having a modal: "File Changed" dialog where the Yes / No buttons don't do anything seems wrong.
I expect that this only happens when I've chosen General > Workspace: [ ] Refresh automatically otherwise I consider it lost functionality i.e. I would no longer be warned if something (maybe unexpectedly) changed my workspace files. This would be a no go for this patch.
(In reply to comment #9) > I expect that this only happens when I've chosen > General > Workspace: [ ] Refresh automatically > > otherwise I consider it lost functionality i.e. I would no longer be warned if > something (maybe unexpectedly) changed my workspace files. This would be a no > go for this patch. Well we would need another preference then. As Refresh Automatically enables the polling provider on non-Windows platforms which is a no-go on slow filesystems. What user benefit is there to knowing a file has changed externally? All this patch changes is user perception. API semantics are unchanged as the resource change listeners are notified and API continues to throw CoreException for out of sync resources. Eclipse is the only app (I know of) that forces users to internalize knowledge of resource sync. Also worth noting that other parts of the workspace silently schedule refresh for parts of the resource tree: - creation of resource filters - creation of links (and therefore aliases are affected) - changing execute bit on folder If we had a preference for this, do you have a use case where you would deliberately disable this functionality?
(In reply to comment #10) > (In reply to comment #9) > > I expect that this only happens when I've chosen > > General > Workspace: [ ] Refresh automatically > > > > otherwise I consider it lost functionality i.e. I would no longer be warned if > > something (maybe unexpectedly) changed my workspace files. This would be a no > > go for this patch. > > Well we would need another preference then. As Refresh Automatically enables > the polling provider on non-Windows platforms which is a no-go on slow > filesystems. OK. > What user benefit is there to knowing a file has changed externally? The file might be changed without the user being explicitly aware of it. This is important information. Otherwise we could have built Eclipse by always forcing the refresh on each file access in the first place. >If we had a preference for this, do you have a use case where you would >deliberately disable this functionality? Yes, I want to know what's going on in my workspace. Other apps/editors also do this i.e. they tell you when the file has changed somewhere else.
I'm happy to add an additional preference if there's consensus this is the way forward. I don't believe this is warranted for these reasons: In CDT and elsewhere API users silently refresh files with #refreshLocal(...). The user is shielded from these out-of-sync warnings. Just doing an open call-hierarchy on #refreshLocal in my workspace, highlights the following in platform: - ide: FileDescription#createExistentResourceFromHandle - ide: WizardNewFileCreationPage#createFile - ide: WizardNewFileCreationPage#createFile - ide: ResourceDropAdapterAssistant#handleDrop - ide: ResourceDropAdapterAssistant#handlePluginTransferDrop - compare: ResourceCompareInput#prepareInput - Text: FileDocumentProvider#doResetDocument - Text: FileDocumentProvider#doSynchronize - Text: FileDocumentProvider#createElementInfo - Text: FileDocumentProvider#handleElementContentChanged - filebuffers: ResourceFileBuffer#revert - many many hits in CDT. - comment 10 + other places in core.resources All of these places perform API #refreshLocal without prompting the user nor reporting out-of-sync. Many of them explicitly eat the CoreException and explicitly #refreshLocal. (In reply to comment #11) > > What user benefit is there to knowing a file has changed externally? > The file might be changed without the user being explicitly aware of it. This > is important information. Otherwise we could have built Eclipse by always > forcing the refresh on each file access in the first place. By this logic there should never be an API call to #refreshLocal that doesn't originate from F5 / user-action. Call-hierarchy shows many such sites. > Yes, I want to know what's going on in my workspace. Other apps/editors also do > this i.e. they tell you when the file has changed somewhere else. They do when the file is open. Eclipse does too though for clean editors you have no choice but to accept the changes: (bug 337435, bug 97510). I've tried to mark these bugs as 'block'-ed by this, as fixing this bug has a direct effect on their impact: Bug 337446 Text search should handle out-of-sync resources Bug 337435 - Don't prompt user when clean editor is about to be brought back into sync Bug 97510 - [misc] auto refresh and external modification detection conflict This bug is also high impact in some NFS environments (read: mine). Here I see files randomly reported as out-of-sync even though I only edit them in the IDE. This seems to be an NFS issue where the modified time stored with the file isn't identical to the time stashed by core.resources. I personally don't think an additional preference is necessary. Why? Because given this out-of-sync information, what can the user do? The only way forward is that she manually refreshes resources before continuing work. If the user is opening an editor it's perverse to require them to refresh manually before they can edit.
Per a twitter exchange with James, I'm commenting here... This is a common problem for developers targeting Android (and I presume others). There are several use cases that are acted on repeatedly through the day that involve pressing F5. 1) Adding new drawable resources (i.e. images). PNG and SVG graphic files that are created by an external editor and dropped into a project in the ./res/drawable folder. 2) Adapting new drawable resource for locale or screen density. This is often "copy/paste/edit" on an existing graphic or "copy/paste/rename" on a folder of graphics. 3) Creating a new locale for translations. The Sequoyah localization editor handles this correctly for the user, but most developers just use the Google plugins. So, when a strings.xml file is intended for a new locale, users often just "copy/paste/rename" the folder. 4) R.java is generated by the Android build scripts and sometimes becomes stale. Typically because the developer did something outside the usual use cases but I don't have a specific use case to prove this. It happens often enough that the commonly accepted workflow is to delete or touch R.java or force it to be updated by modifying a layout or string just enough to force the generation. I'm not sure if this one is in scope or not.
Just ran into this now doing a text search. Eclipse knows the file is out of sync. Sync it then. Why is this my problem? QED.
After reading over this bug again and sleeping over it I'm convinced that this is not a good idea. Let me explain why: 1. It does not solve the problem. The fix still lets the first access to out-of-sync resource fail. Hence, it only helps those parts of the system that act on the resource change event that comes shortly after the resource gets refreshed. Other parts of the system, like e.g. search, actions, refactorings etc. are not fixed. In order to get the behavior that you desire, client code has to be touched, as the bugs (see bug 337446, bug 337435) nicely proof. Of course those are just the tip of the iceberg because there's Java search, PDE search, etc. While CDT chose that to automatically refresh, this is not the policy in the SDK and not the story that was told to other plug-in providers. Therefore there's lots of code out there that would still not act as desired and would have to be fixed in order to behave as expected. As you can imagine most projects are on tight schedule and they won't have time to do that work. 2. The fix can surface problems in existing code. With the existing auto-refresh preference we already have the situation where a message is shown to the user that something is out-of-sync but when he continues the resource has been automatically refreshed. With this fix such situations will occur more often (as correctly noticed in bug 337435) and I fear that there are many other places where this would surface strange UI interactions. 3. It might impact performance (just a guess). Let's assume you want to build a project has tons of files that are out- of-sync: each time when the builder hits such a file it would get an exception and later it gets started again due to a resource delta(s). Having said that, I understand that there are scenarios, like in CDT, where the current out of the box behavior regarding out-of-sync does not fit well and that the auto-refresh preference cannot be enabled due to performance reasons. In my opinion the correct fix that I supported already before and that will not require any client fixes (very important) is to fix bug 14867 once and for all. I think that will be the right thing to do or did I miss anything?
Thanks Dani, I agree with your points. (In reply to comment #15) > In my opinion the correct fix that I supported already before and that will not > require any client fixes (very important) is to fix bug 14867 once and for all. > I think that will be the right thing to do or did I miss anything? Most the discussion in bug 14867 seems to involve refresh. Given your constraints 1,2+3 the only way I can see fixing this is to change getContents() so that it no longer throws CoreException on out-of-sync. We would still need to schedule refresh in the background so listeners are notified of the change. Is this what you had in mind? The existing contract on getContents() gives guarantees that the resource has't been modified externally. However this isn't as useful as it might seem, as the API gives no guarantees about changes made concurrently within Eclipse. Yes a resource delta is guaranteed to fire 'at-some-point' after a resource change is made. However 'at-some-point' isn't well-defined, especially with batched changes, so today file readers may getContents before listeners have been notified that the file has been changed. Changing the semantics of getContents in this way makes external changes appear as if the external modification were a concurrent write occurring just before the read within the eclipse platform.
> Most the discussion in bug 14867 seems to involve refresh. Yes, it took that turn but it's not exactly what I had in mind (see below). > Given your constraints 1,2+3 the only way I can see fixing this is to change > getContents() so that it no longer throws CoreException on out-of-sync. We > would still need to schedule refresh in the background so listeners are > notified of the change. Is this what you had in mind? Almost. I think the only way to fix this without client fixes (like in search) is to refresh directly when it's out-of-sync and return the refreshed data, otherwise the fix for bug 14867 has the same issues as discussed here.
Ah, i dont think refresh in the read path would be possible. The WS get operations dont acquire WS lock and scheduling rules as required by the modif. apis. But that's ok. All refresh does is build a resource delta and notifies listeners. We can still return the file input stream -- sync state + delta will update asynchronously and likely concurrently.
Created attachment 189549 [details] fix + tests 2 Changes as per comment 16. Change API of #get... to not throw CoreException on out-of-sync. Resource is reported as changed in a subsequent resource change notification asap after discovery. Details: 1) IFile#getContents() changed to to IFile#getContents(true) no longer throw CoreException for out-of-sync. 2) IFile#getContentDescription() doesn't throw CoreExcetpion on out-of-sync. Similar to (1). Returns cached content description if in-sync, or actual content description if out-of-sync. Resource-delta fired shortly after. In fixing 2, beefed up the test for bug 186984 with scenario of Dani's: bug 186984 comment 8. Discovered bug where an incorrect timestamp is stashed when updating the content description cache.
(In reply to comment #19) > Created attachment 189549 [details] [diff] > fix + tests 2 > > Changes as per comment 16. Change API of #get... to not throw CoreException on > out-of-sync. Resource is reported as changed in a subsequent resource change > notification asap after discovery. > > Details: > 1) IFile#getContents() changed to to IFile#getContents(true) > no longer throw CoreException for out-of-sync. > 2) IFile#getContentDescription() doesn't throw CoreExcetpion on out-of-sync. > Similar to (1). Returns cached content description if in-sync, or actual > content description if out-of-sync. Resource-delta fired shortly after. Direction looks good to me but I assume that this only happens when a certain preference is set (sorry don't have time right now to dig into the patch). As I tried to explain earlier, the existing behavior that people are used to for almost ten year now cannot be changed but a new option to always fetch the latest should be added.
(In reply to comment #20) A couple points: - #getContentDescription only started throwing CoreException on out-of-sync in 3.4 - #getContents has been doing so for much longer > Direction looks good to me but I assume that this only happens when a certain > preference is set (sorry don't have time right now to dig into the patch). As I > tried to explain earlier, the existing behavior that people are used to for > almost ten year now cannot be changed but a new option to always fetch the > latest should be added. The issue is I don't see when someone would ever turn the preference off. Can you give an example of how the new behaviour could cause problems requiring the disabling of the preference? Eclipse already has many preferences, and I can't see when someone would ever turn this one off.
(In reply to comment #21) > (In reply to comment #20) > > A couple points: > - #getContentDescription only started throwing CoreException on out-of-sync > in 3.4 > - #getContents has been doing so for much longer > > > Direction looks good to me but I assume that this only happens when a certain > > preference is set (sorry don't have time right now to dig into the patch). As I > > tried to explain earlier, the existing behavior that people are used to for > > almost ten year now cannot be changed but a new option to always fetch the > > latest should be added. > > The issue is I don't see when someone would ever turn the preference off. Can > you give an example of how the new behaviour could cause problems requiring > the disabling of the preference? Eclipse already has many preferences, and I > can't see when someone would ever turn this one off. I think I tried to explain it several times. Like you have your very valid use case (that you presume to be the right/default one), others like me want to be informed if anything changes outside the workspace. That was also the decision made originally and that people are used to for years now. I can understand that from your perspective you can't imagine this being off. From my perspective I can't imagine that anyone ever changes a workspace file outside the workspace. ;-)
(In reply to comment #22) I don't mean to labour this, but... I'm trying to understand any unforseen (by me) ramifications of the change. In particular I'm keen to focus on the change in API contract and whether there is any *real* loss of information / functionality for callers. Concretely, if there were a preference I would want it to be on-by-default; who would this affect? The change: - IFile#getContents() no longer throws CoreExcpetion for out-of-sync. Client behaviour: - Clients generally expect #getContents() to succeed. Any failure condition must be reported or handled. For out-of-sync there are two ways to handle: 1) Display error to user + force user interaction 2) Silently refresh file + ignore error - Assertion: Clients can't glean any useful information from out-of-sync on #get...: (Out-of-sync can be thrown without external modification (there is no read locking the WS, so it's possible for this sequence: file-modified; getContents; sync-updated) From user's POV: - If we remove out-of-sync on #getContents, then yes users may not be aware of external modification. However users _already_ unaware of any within IDE file modifications. I.e. any API modification using #setContents not made by the user themselves in the editor. Why should the user care any more about external modification than in-IDE modification? If there is external modification, the user finds out after-the-fact. From API caller POV: - isSynchronized() will still report if resource is out-of-sync. - #set... API still throws Exception on out-of-sync. - #get... API doesn't throw on out-of-sync, one less error condition to handle. This makes external modifications behave similarly to concurrent modifications within the IDE. So key points: 1) API callers can still check sync state. 2) Editor UI _already_ checks #isSynchronized => displays an out-of-sync modal dialog... Editors are the way users today find out about out-of-sync. So with this patch, and without any preferences, users will still hear about out-of-sync. The last point is key. Users will still find out about out-of-sync from the editor if the UI team think this is important. With or without a workspace preference the same information is available by default in today's IDE! By all means we could have a preference, but it shouldn't be in core.resources...
(In reply to comment #22) > From my > perspective I can't imagine that anyone ever changes a workspace file outside > the workspace. ;-) FWIW, if I remember correctly, the decision that Eclipse should work with files and folders in the file system (and not with some internal representation of source files like in previous OTI IDEs) was made so that external tools can be used alongside with the in-process plug-ins. We have a workspace that can be out of sync with the file system, because the Java file system APIs were not sufficient to implement a well-performing incremental Java compiler which needs resource delta notifications. If it wouldn't potentially cause performance problems, we should have turned on auto-refresh by default long ago.
(In reply to comment #24) > We have a workspace that can be > out of sync with the file system, because the Java file system APIs were not > sufficient to implement a well-performing incremental Java compiler which needs > resource delta notifications. Agreed, and it's important that strong model of WS resources is maintained -- listeners are notified of the external change as soon as we discover out-of-sync. > If it wouldn't potentially cause performance problems, we should have turned on > auto-refresh by default long ago. +1. This approach has no additional overhead for the usual case of the WS remaining in sync. The point of contention seems to be that Dani likes the existing behaviour and wants to know if a file is ever modified outside of Eclipse. I'm trying to work out if this information really is essential to his workflow and particularly whether this warrants an additional preference.
(In reply to comment #25) > The point of contention seems to be that Dani likes the existing behaviour and > wants to know if a file is ever modified outside of Eclipse. I'm trying to > work out if this information really is essential to his workflow and > particularly whether this warrants an additional preference. As a general statement regarding all the resource sync related bugs that have been flying around lately: To me, resources being out of sync is really just an error condition. Unless you are trying to debug the resource system itself, it's not really useful information that the user cares about or is going to act upon in any meaningful way other than to just refresh the resources to get them in sync so that the IDE stops complaining. The way it is now, they ostensibly can't do anything useful with the resource until it's in sync, because errors get thrown all over the place, and the only operation they can perform is the operation to correct the condition, i.e. refresh the resource. If that's all we're going to let them do and it's the only way for them to fix the problem I really don't see why it doesn't make sense to just automate it for them and fix the problem. Spun another way... who is really counting on the current behaviour? I don't think anyone (except perhaps in core.resources.tests) is deliberately somehow causing resources to be out of sync and then absolutely relying upon operations later failing on them. Operations on the resources generally are executed expecting success, and failures are handled should they happen to occur. I don't think anyone will complain if the number of ways an operation can fail suddenly goes down provided that the end result behaviour that the user expects remains correct. We've had lots of people complain that they don't want this message and want things to "just work" but really only one person has argued that they want to see the information this error is providing. If we are able to make it so this error condition happens less, or is automatically corrected, then it makes the lives of lots of users easier, and possibly makes the life of one developer (Dani) harder. If we really want to see it every time it happens for debugging purposes then by all means it is useful to allow the behaviour to be turned on via a debugging flag or somesuch, but so far it seems like the only person that wants to see these problems is Dani, and no offence to him, but our target audience is a lot larger than just him. If there are other users or downstream clients that truly want to see the current behaviour, I would really encourage them to speak up, but I highly suspect they are few, if they exist at all.
Hi, I am having some difficulties tracking all the comments especially that I am not familiar with eclipse code. I am insterested in the behaviour to the user: Current behaviour is that during project refresh, changed (edited) files cannot be saved. Does this mean that during editing of a file, I cannot save it because some files are refreshed automatically? In big projects refresh could take 10 minutes, sometimes more? Thanks, Yevgeny
From user's point of view there's really 2 scenarios / workflows: (A) "Workspace is king" (Dani's expectation and default Eclipse Preferences) (B) "Filesystem is king" (Which most users that I know intuitively expect). Up to now, case (B) was only supported by the Refresh Monitor which works well on Windows and not so well on other OS's since it's polling. Now, we're adding a new feature in support of (B), refreshing not by timer but on accessing resources. I support the idea of adding a Preference into the UI on the "Workspace" page, but I'm not yet sure how to best express these basic usage scenarios and the supporting features. One idea could involve renaming the existing feature: Refresh Automatically [x] On timer or OS events [x] On resource access Along with documentation explaining that "Refresh Automatically" is what you want when you expect the file system to be king, and mentioning that this autorefresh may have side-effects such as event flooding or unexpected resource deltas.
(In reply to comment #28) > Up to now, case (B) was only supported by the Refresh Monitor which works well > on Windows and not so well on other OS's since it's polling. Now, we're adding > a new feature in support of (B), refreshing not by timer but on accessing > resources. There are filesystem event APIs on OSX: http://developer.apple.com/library/mac/#documentation/Darwin/Conceptual/FSEvents_ProgGuide/Introduction/Introduction.html
(In reply to comment #27) > I am insterested in the behaviour to the user: Current behaviour is that during > project refresh, changed (edited) files cannot be saved. Does this mean that > during editing of a file, I cannot save it because some files are refreshed > automatically? In big projects refresh could take 10 minutes, sometimes more? This happens because you've selected a large project or set of resources for refresh, and the refresh operation (from the UI) does it all in one go. The changes proposed here are working at the File level, not folder or project level. Also if there are many files out of sync (discovered as part of a file search, for example) then the 'refresh' is broken into smaller pieces so as not to impede the user. (In reply to comment #28) > I support the idea of adding a Preference into the UI on the "Workspace" page, > but I'm not yet sure how to best express these basic usage scenarios and the > supporting features. One idea could involve renaming the existing feature: > > Refresh Automatically > [x] On timer or OS events > [x] On resource access > > Along with documentation explaining that "Refresh Automatically" is what you > want when you expect the file system to be king, and mentioning that this > autorefresh may have side-effects such as event flooding or unexpected resource > deltas. Iit's those side effects that are key to this discussion. The longer the workspace remains out-of-sync with the filesystem the longer the user has incorrect build output, and an incorrect view of the world, etc. We already discover the file is out of sync during resource access, so why don't we act on it and resolve the situation? When would you expect the user to disable "On resource access"? "and mentioning that this autorefresh may have side-effects such as event flooding or unexpected resource deltas" <- this is scary. We can't possibly expose this to the user and expect them to make a sensible decision. Resource deltas must fire on changed resources for eclipse to function correctly. If there's 'event-flooding' then that would be a bug. Fundamentally core.resources API allows writes from any thread and promises to update listeners in a timely manner (possible batched). By ignoring out-of-sync resources we're breaking that contract and listeners are unaware that there are external changes they may need to reconcile. IMO we shouldn't give users the option to perpetuate this...
(In reply to comment #29) > There are filesystem event APIs on OSX: > http://developer.apple.com/library/mac/#documentation/Darwin/Conceptual/FSEvents_ProgGuide/Introduction/Introduction.html Feel free to write and contribute a refresh provider for OSX ... the API's are all public, but so far a provider only exists for Windows. I'm sure your contribution would be well received: http://help.eclipse.org/helios/topic/org.eclipse.platform.doc.isv/guide/resAdv_refresh.htm I know there's been discussions in the past about supporting the fam service on Linux as refresh provider, but I don't have a bug number at hand nor do I know how far this got.
(In reply to comment #30) > We already discover the file is out of sync during resource access, so why > don't we act on it and resolve the situation? > When would you expect the user to disable "On resource access"? As Dani wrote: It's a valid usage scenario to expect that the Workspace is king and any outside modification of files is unexpected and potentially dangerous so the user would expect to be warned about it. It might be a minority of users thinking along these lines, but it's valid.
(In reply to comment #32) > As Dani wrote: It's a valid usage scenario to expect that the Workspace is king > and any outside modification of files is unexpected and potentially dangerous > so the user would expect to be warned about it. > > It might be a minority of users thinking along these lines, but it's valid. Right, so now you know about it. You decide you'd like to get some work done, you have two options to proceed: a) close your eclipse editor and go back to emacs b) find the file in a resource navigator, right click > refresh* ** In my mind a better preference name would be: - On discovery out-of-sync file: Display modal dialog + require user intervention to manually refresh file. Ignoring whether the user is aware of this, do we agree that the overall semantics of the API to core.resources clients is the equivalent? If we agree with this, then the preference can be in the UI editor, not on core.resources. The important thing to remember here is the: *user double clicked on a file*. They want to see its contents. Sync state is an artifact of how the WS manages resource deltas and maintains consistency. Where possible (particularly on read) users shouldn't be exposed to this. API Clients that want to know still can know: use #isSynchronized() or #getContents(false). * for some reason F5 hasn't worked for the last few releases on Linux ** some views (such as the ClearCase integration) don't have the refresh action. So you have to change perspective, hunt for the file in a navigator view and refresh.
We are really talking about two different things here: 1) Automatically refresh. This should bring the whole workspace in an up-to-date state with the file system and that's what we ideally would like to have but isn't possible to achieve with good performance on all OSs. That's why some people don't enable that preference (besides the fact that the refresh is not happening instantly). 2) Get the current information/contents when accessing a file (this bug). This will not solve 1) but reduce the number of out-of-sync cases when accessing a file. The drawback of this new comfort will be: - The user might no longer know that his workspace (e.g. added or deleted files and folders) is out of sync because he no longer gets any notification. So, the point that was made before in comment 30 about knowing earlier that the workspace is not in sync actually becomes worse. - Operations that previously didn't do anything but fail will now cause resource deltas and trigger work being done. Clients might not be prepared to that or even expect that they get the exception when out-of-sync. I would offer the following preferences: [ ]Refresh Automatically [ ] Refresh files on access 'Refresh files on access' would automatically be enabled if the parent preference is checked.
(In reply to comment #33) Perhaps it helps if I give an example. Assume I'm working on a CVS-controlled workspace, editing all my sources in Eclipse, and running an external "make" tool to translate my code. Due to a silly bug when editing my Makefile, I messed up my build. Rather than copying "fooCustomer.c" from src1/ into obj1/ I now copy that file from src1/ into src2/ when building. Little later I want to edit src2/fooCustomer.c . The resource is auto-refreshed so I edit the variant that's a copy from src1/ rather than the original one from src2/ . The variants are quite similar so I don't notice and commit my change. Bad bug introduced.... QED. That's why Dani and I think the "Workspace is King" workflow is valid.
>Ignoring whether the user is aware of this, do we agree that the overall >semantics of the API to core.resources clients is the equivalent? If we agree >with this, then the preference can be in the UI editor, not on core.resources. This is not just about editors as you know yourself having filed the same issue against search. Having said that, we can't change the default behavior of APIs out of the box. If the PMC allows such an API change then it must at least be optional 1. from a user perspective and 2. from an API client perspective, e.g. RCPs that use the API and have certain expectations based on the current API contract.
(In reply to comment #35) > (In reply to comment #33) > > Perhaps it helps if I give an example. > ... > Little later I want to edit src2/fooCustomer.c . The resource is auto-refreshed > so I edit the variant that's a copy from src1/ rather than the original one > from src2/ . The variants are quite similar so I don't notice and commit my > change. > > Bad bug introduced.... QED. Ok. And exactly the same thing can happen without leaving the workspace: - You accidentally have an external Program builder that runs 'cp' to the wrong place and refreshes resources (and you would have refresh because that's the only way to get eclipse to notice the changes...) - You accidentally put source files in an output directory and Eclipse helpfully deletes them for you (Bug 335435) - You have some badly behaved plugin that accidentally overwrites files. - You have one of the number of call-sites eating CoreException or implicitly refreshing files (comment 12) What I don't get is why changes within the IDE (not by the editor) are OK, but changes outside the IDE aren't? Users who have this problem, almost certainly know about the external changes they made -- they made them deliberately. They have no idea what secret resource changes happen by virtue of running Eclipse. I don't see why one is any more preferable to the other. (In reply to comment #36) > Having said that, we can't change the default behavior of APIs out of the box. > If the PMC allows such an API change then it must at least be optional 1. from > a user perspective and 2. from an API client perspective, e.g. RCPs that use > the API and have certain expectations based on the current API contract. Irony is this was precisely what was done in bug 186984 when #getContentDescription() was made to throw an exception on out-of-sync. This change removes a possible failure case to the method, and I disagree with your assertion that this changes API in any meaningful way. Clients already need to cope with concurrent change in the workspace as per comment 23.
(In reply to comment #36) > This is not just about editors as you know yourself having filed the same issue > against search. That's precisely my point. I'm asserting that the editor is one of only a few call-sites which face the user and therefore may 'care' about this. If you decide users must know, then use #getContents(false) or #isSynchronized() and tell them! However to be fair the editor should report out-of-sync for *any* file change which wasn't initiated by the user... From my POV you have no idea who's modifying resources using the core.resources API vs who's modifying resources outside of Eclipse. Why single out only one of these resource modifiers for special treatment?
(In reply to comment #35) > (In reply to comment #33) > Perhaps it helps if I give an example. > Assume I'm working on a CVS-controlled workspace, editing all my sources in > Eclipse, and running an external "make" tool to translate my code. > Due to a silly bug when editing my Makefile, I messed up my build. Rather than > copying "fooCustomer.c" from src1/ into obj1/ I now copy that file from src1/ > into src2/ when building. > Little later I want to edit src2/fooCustomer.c . The resource is auto-refreshed > so I edit the variant that's a copy from src1/ rather than the original one > from src2/ . The variants are quite similar so I don't notice and commit my > change. > Bad bug introduced.... QED. > That's why Dani and I think the "Workspace is King" workflow is valid. I think this is a bad example. I don't think that use case is at all likely and I'm not sure why the IDE is supposed to guard against that type of problem. If I did something wrong outside of Eclipse, it's not Eclipse's responsibility to protect me from myself. If I were to open Emacs on the same file it would happily just load up what's on the disc and I wouldn't expect any different. Users are going to expect that Eclipse behaves similarly.
(In reply to comment #34) > [ ]Refresh Automatically > [ ] Refresh files on access > 'Refresh files on access' would automatically be enabled if the parent > preference is checked. Due to the performance concerns with Refresh Automatically, I don't think the two preferences ought to be tied to each other. I would want to be able to refresh on access without having Refresh Automatically polling in the background all the time.
(In reply to comment #39) > I think this is a bad example. I don't think that use case is at all likely > and I'm not sure why the IDE is supposed to guard against that type of problem. I disagree. In the "workspace is king" philosophy, my CM system and my own editor are the only ones that can change content of any source files in the workspace. I trust the workspace system (plus internal builders etc) to be safer than the outside world in the filesystem. FWIW, in our own product on top of Eclipse we'd certainly turn on a Preference to enable the automatic refreshes, by using plugin_customization.ini. But conceptionally, I tend to agree with Dani that the IDE + Workspace is "more managed" and can thus be considered "safer" for unwanted file modifications than the file system.
(In reply to comment #41) Martin I'd encourage you to try the patch. If you encounter or can dream up circumstances where you would actively disable the preference then it may be worth the effort. There seems little point in going through the effort if no one will turn it off.
(In reply to comment #41) > I disagree. In the "workspace is king" philosophy, my CM system and my own > editor are the only ones that can change content of any source files in the > workspace. I trust the workspace system (plus internal builders etc) to be > safer than the outside world in the filesystem. This might be a nice in theory, but it's only as a good as how true it is. Open call-hierarchy in an IDE workspace on refreshLocal... There are very many places in UI, Compare and CDT that call this. The big exception is the editor, hence all the complaints. There are a couple more reasons for not putting this in the hands of the user... 1) Complicates the API. - If there's a preference then this API has two behaviours. throw and not throw CoreException. Callers need to handle both. Currently they only have to handle one, and they do so inconsistently. 2) Increases the chance of resource corruption. - If we let out-of-sync resources languish, then caches (such as IFile#getCharset) retain stale data (bug 209167). This could lead to silent corruption (e.g. FileDocumentProvider#doSaveDocument calls saveContents with force sometimes true & it uses #getCharset which doesn't check sync!) . If instead a delta is fired after discovering out-of-sync, then this and other caches could be invalidated, and all interested parties can correctly update their view of the world.
(In reply to comment #41) > I disagree. In the "workspace is king" philosophy, my CM system and my own > editor are the only ones that can change content of any source files in the > workspace. I trust the workspace system (plus internal builders etc) to be > safer than the outside world in the filesystem. FWIW, I use other tools that are external to the IDE, and have no desire to integrate them. So I'm in the "filesystem is king" camp. I run with "refresh automatically", but it isn't quite good enough. I was just bitten by an out-of-sync: I forgot to do a refresh and received an OOS as my debug session entered the file -- resulting in the termination of my debug session. Argh! I find the out-of-sync issues to be a headache.
There are so many users begging wanting the new preference to "refresh the resource on discovery of out of sync automatically". Overwhelming majority of users want it, so please make that the default one. That kind of things sure can be done in new release, and negatively affected are few.
(In reply to comment #40) > (In reply to comment #34) > > [ ]Refresh Automatically > > [ ] Refresh files on access > > 'Refresh files on access' would automatically be enabled if the parent > > preference is checked. > > Due to the performance concerns with Refresh Automatically, I don't think the > two preferences ought to be tied to each other. I would want to be able to > refresh on access without having Refresh Automatically polling in the > background all the time. Well, the idea is to tie them the other way around i.e. you can check 'Refresh files on access' while 'Refresh Automatically' is unchecked, but if 'Refresh Automatically' is checked, then of course 'Refresh files on access' will be selected automatically.
Created attachment 189685 [details] fix + tests 3 Ok, I relent. Changes: - core.resources patch to add a preference: PREF_FS_REFRESH to complement existing: PREF_AUTO_REFRESH - PREF_AUTO_REFRESH predicates all the automatic core.resources refreshing of discovered out-of-sync resources. This now defaults to true. - PREF_FS_REFRESH uses external filesystem (or polling) refresh monitors. This defaults to false. IMHO users should never disable PREF_AUTO_REFRESH (or bad things might happen: comment 43). ide patch coming.
Created attachment 189686 [details] UI IDE patch Add additional preference so users can control Filesystem refresh monitors independently from automatic refresh: Name: Refresh filesystem automatically Tooltip: Automatically refresh the filesystem to discover changes.\nPolls periodically where a FS notification hook doesn't exist. Selecting this preference automatically selects the auto-refresh preference.
I'd appreciate feedback on the new patch, especially from anyone who had concerns about the previous one.
Sheesh, I go away for a couple of days, and look at all the fun I miss! Here's my input: - I like the idea of this being tied to the "Refresh automatically" mode. Essentially when in this mode, any information we can use to ensure timely *asynchronous* refresh is fair game. - I don't like the idea of having two preferences to paper over a perceived performance problem with polling. From a user perspective, they either want Eclipse to always be in sync with the file system, or they don't. I don't think any user will be able to evaluate the polling overhead on their system in order to decide what is the best value for that setting (ok, other than James ;). Personally I have been working for a few months with autorefresh enabled, on a system with no native refresh hooks, and have not observed any performance impact (it just takes the system awhile to discover changes). The polling implementation is coded in a such a way that it backs off if polling takes too long. I would be inclined to make this code smarter if necessary rather than add a preference. For example, if it exceeds some time threshold it could decide to give up polling entirely, or only poll once an hour, etc. The polling code is in a much better position to figure out whether it is useful, rather than imposing this decision on the user with yet another preference. So, that's my general input. Tie this to a single "refresh automatically" mode, and use every trick available when in this mode to bring things into sync as quickly as possible. If we can make this mode useful and non-invasive on all platforms we can then consider turning it on by default.
(In reply to comment #43) > There are a couple more reasons for not putting this in the hands of the > user... Hm... James, you have presented some very compelling arguments. Couple thoughts at this point: - As Boris has pointed out, Eclipse did make a decision to honor the file system in order to support external builders, from its very inception. - The vast majority of users is expected to be in the "filesystem is king" camp. - Very few end users will understand a UI Preference or ever deliberatly modify it. Your comments about "internal" plugins in the Eclipse ecosystem being able to cause unexpected workspace modifications made me think. Essentially, it looks like the "workspace is king" scenario can only be guaranteed in a closed system where all plugins are known to behave well (eg bundled/tested by a product builder). This makes my argument about misbehaving external tools less strong. So what remains is concern about backward compatibility. I'm considering the idea of having a non-UI Preference plus note in the migration guide, such that product builders could re-enable the old style behavior if need be, but the default would be your new style behavior. I'm still wondering whether we could dream up some worst-case scenarios with the new behavior. For instance, imagine somebody copies a Workspace from A to B, and the copy operation modifies all timestamps. The workspace is not auto-refreshed on startup, A CM provider (eg Subversive) happens to notice lots of out-of-date resources in its internal data, leading to an event storm. Would refresh notifications be coalesced into few deltas? Long story short, - I could certainly live with a new UI Preference for the new feature (but of course semantics of the old Preference must remain the same on API level and same or very very similar on UI level... think about existing product docs which describe the "Refresh Automatically" Preference). - Just having a non-UI Preference + migration docs might be preferrable - Have we thought about all potential worst cases with the new behavior? As John says, if Performance / usability concerns for a general "Refresh Automatically" default have become moot over time, this might be the best solution.
(In reply to comment #50) > - I don't like the idea of having two preferences to paper over a perceived > performance problem with polling. From a user perspective, they either want > Eclipse to always be in sync with the file system, or they don't. Unfortunately this hoses NFS filers here. We can get 90% CPU utilization on the filer on bad days with lots of Eclipses... So we really need to decouple FS polling from async-refresh. On RHEL4 with NFS, there's a kernel bug where the linux client leaks inode and dcache entries that causes the kernel to eventually grind to a halt. 10 or so Eclipses on a machine can bring it down in a few days. (There are workarounds for this, but excessive stat()ing on a network fs is pretty anti-social.) I've also seen it back off to an interval that's multiples of hours. The back off is very pessimistic: //make sure it doesn't run more than 5% of the time long delay = Math.max(MIN_FREQUENCY, (System.currentTimeMillis() - time)* 20); If the Poll is severely I/O bound then this uses much much less than 5% of CPU time. > So, that's my general input. Tie this to a single "refresh automatically" mode, > and use every trick available when in this mode to bring things into sync as > quickly as possible. If we can make this mode useful and non-invasive on all > platforms we can then consider turning it on by default. Ok. I think in big enterprise this can hurt filers, so we want some way to control this. Do you have any feeling on whether we could, by default, async refresh files when we discover them out-of-sync? There are issues with letting staleness linger (comment #43) and many places in the SDK refreshLocal's silently anyway (comment #12)
(In reply to comment #51) I agree with your points. > I'm still wondering whether we could dream up some worst-case scenarios with > the new behavior. For instance, imagine somebody copies a Workspace from A to > B, and the copy operation modifies all timestamps. The workspace is not > auto-refreshed on startup, A CM provider (eg Subversive) happens to notice lots > of out-of-date resources in its internal data, leading to an event storm. Would > refresh notifications be coalesced into few deltas? This would be ok. The changes incur no additional work unless the API call deliberately checks sync-state. Taking your case above: - Iterating over a WS tree uses the cached data. (no change) - It's not until the client explicitly uses an operation that checks sync that the resource is added to the refresh job's queue and a refresh is scheduled. This concretely affects: - calls #getContents, #getContentDescription on IFiles - #isSynchronized on IResources - #set... methods (which still throw CoreException on out-of-sync.) When iterating over the tree I'd assume that the client isn't opening an input stream on the files... Iterating over the WS doesn't check sync state of resources. The RefreshJob batches change notification and attempts to minimize workspace locking during the refresh. If the existing refresh job algorithm has issues we can fix that separately. If the client uses #isSynchronized, this works as before. #isSynchronized aborts and returns false on the first resource it discovers out-of-sync. This will schedule at most one resource (per-call) for refresh. In summary we'll only schedule additional work when we've performed an operation that has performed I/O. This work will only happen for the 'rare' out-of-sync case. Operations which operate on cached state continue to do so. > Long story short, > - I could certainly live with a new UI Preference for the new feature (but of > course semantics of the old Preference must remain the same on API level and > same or very very similar on UI level... think about existing product docs > which describe the "Refresh Automatically" Preference). > - Just having a non-UI Preference + migration docs might be preferrable The existing preference describes precisely what we're doing ;) -- refreshing automatically. On the other hand we need some way to disable unnecessary expensive prophylactic I/O. By all means we could add a hidden new on-by-default preference for this behaviour, if people would be happy with that?
(In reply to comment #52) > Unfortunately this hoses NFS filers here. We can get 90% CPU utilization on > the filer on bad days with lots of Eclipses... So we really need to decouple > FS polling from async-refresh. Interesting. So to be clear the performance problem manifests on the file server, not the machine where eclipse is running (I'm assuming "filer" means file server). > On RHEL4 with NFS, there's a kernel bug where the linux client leaks inode and > dcache entries that causes the kernel to eventually grind to a halt. 10 or so > Eclipses on a machine can bring it down in a few days. (There are workarounds > for this, but excessive stat()ing on a network fs is pretty anti-social.) Yes. What I'm wondering is if there is any way we can detect this and have the polling system turn itself off. I imagine there are many users out there that haven't discovered Eclipse is behaving badly in such an environment, and I consider it a bug that we don't behave well in all environments. > I've also seen it back off to an interval that's multiples of hours. The back > off is very pessimistic: This part doesn't bother me. If the polling is too slow to be useful, it isn't doing any harm. Your changes adds another technique to the arsenal that should make auto-refresh perform better in such cases. > Ok. I think in big enterprise this can hurt filers, so we want some way to > control this. Agree. I was just searching for a way that we can detect this, because it seems like a cop-out to just introduce a preference and leave it up to users to tailor preferences to get eclipse working properly in their environment. A small number of power users will figure this out, and the rest will just live with Eclipse behaving badly. > Do you have any feeling on whether we could, by default, async refresh files > when we discover them out-of-sync? > There are issues with letting staleness linger (comment #43) and many places in > the SDK refreshLocal's silently anyway (comment #12) I don't like the idea of refreshing automatically when the "refresh automatically" preference is disabled, if that's what you're asking.
(In reply to comment #54) > Interesting. So to be clear the performance problem manifests on the file > server, not the machine where eclipse is running (I'm assuming "filer" means > file server). Yes, we see high load average on the NetApp filer. It's not clear to me at what points the local machine uses cached state vs. going over the network. However, a full WS refresh is quite pessimal, as for many users large chunks of the fs either need to be cached or fetched from the file-server. Thing is while NFS is bad something like ClearCase will be much much worse... > Yes. What I'm wondering is if there is any way we can detect this and have the > polling system turn itself off. I imagine there are many users out there that > haven't discovered Eclipse is behaving badly in such an environment, and I > consider it a bug that we don't behave well in all environments. This is interesting. I guess the issue is that NFS load varies throughout the day depending on what other work is happening on the network. Eclipse is implicated, but by no means the only culprit. Perhaps we could do something clever like time I/O and keep a running average. If the time taken deteriorates, back off. Need to be careful that the cost of tracking this doing this might be prohibitive though? > I don't like the idea of refreshing automatically when the "refresh > automatically" preference is disabled, if that's what you're asking. Ok. There's also Martin's idea of a hidden preference that products can control. I guess the issue I've got is that the PollingMonitor is nearly always just burning cycles / I/O. This patch is much more targeted. We're only refreshing actually out-of-sync resources. Lumping them under the same preference is very coarse. Moreover the refresh monitor stuff needs work - frequency is a concern (bug 265149), delayed-populated containers are a problem (bug 321683), linked resources are an issue (bug 265125). I'm keen to get this in as it'll make users' lives much better. - If we predicate this on "Refresh Enabled", will that keep everyone happy for now? - Does anyone object to ticking "Refresh Enabled" by default?
Created attachment 189752 [details] fix + tests 4 - All Auto-Refresh uses the single auto-refresh preference - Auto-refresh preference enabled by default.
Comment on attachment 189752 [details] fix + tests 4 Acctually scratch that. This feature shouldn't be tied to the expensive polling provider. This patch solves user complaints without incurring unnecessary I/O overhead. I think this should be on by default, and the existing preference should still control the filesystem hook / polling provider (as it does now). If we really need to allow people to disable this functionality then the only option would seem to be another preference.
Created attachment 189780 [details] fix + tests 5 Add a preference to core.resources: public static final String PREF_REFRESH_ASYNC = "refresh.async.enabled"; //$NON-NLS-1$ allows products to disable this new functionality and restore the previous behaviour. So there are now 3 distinct options: - fix + tests 1 = read API continues to throw CoreException, but schedules async refresh unconditionally - fix + tests 2 = read API doesn't throw CoreException and schedules async refresh unconditionally - fix + tests 3 = add a UI preference to control auto-refresh vs. filesystem refresh - fix + tests 5 = add a hidden core.resources preference which products can use to disable this auto async-refresh. Refresh automatically preference continues to control the expensive polling monitor. I think v5 gives a good balance. We, or other products, can easily revert the functionality with a single boolean. I'm keen to get this in before M6 so we can get exposure and feedback from across the community. Realistically this means we have a week (less if we want to get it in for next week's I-Build). As an aside, our egit friends have just today added another silent #refreshLocal to workaround out-of-sync (no prodding from me, honest!): http://egit.eclipse.org/r/2596
Thanks, James, for all your hard work on this! I think that "fix + tests 5" is a balanced pragmatic approach and probably the best we can do at the moment. It's not really clean (since we start refreshing automatically on certain occasions although the preference is off); but, pragmatically speaking this is happening across the place with silent #refreshLocal ala egit already, it clearly improves user experience and it is likely what the majority of users will want. +1 for this approach for now and in M6, since it will allow us to use this in practice, find potential issues, and still revert the default at any time if need be. A cleaner solution would tie all "refresh automatically" algorithms together under a single Preference (as John suggested). But that would involve fixing the polling monitors and even then we wouldn't be protected against silent #refreshLocal from plugins. Another option would be UI Preferences as follows: [x] Refresh automatically [x] Allow file system polling Where we would turn on the "Refresh Automatically" by default to enable your feature plus potentially any non-polling refresh monitors, plus allow users to walk the extra mile and even allow polling if they want. I find this approach also compelling, but not as compelling as "fix + tests 5", because this approach (a) re-interprets the meaning of an existing UI preference which people are used to, which is similar to API breakage, and (b) still isn't 100% exact due to contributor plugin's silent refreshLocal. But I could certainly live with the extra UI preference if others like it (Dani, you want the "workspace is king" scenario exposed in the UI?)
(In reply to comment #59) > Another option would be UI Preferences as follows: > [x] Refresh automatically > [x] Allow file system polling > Where we would turn on the "Refresh Automatically" by default to enable your > feature plus potentially any non-polling refresh monitors, plus allow users to > walk the extra mile and even allow polling if they want. That's precisely 'fix + tests 3'. Auto-refresh controls low-impact async refresh. And an extra preference is provided for more expensive fs hooks. > But I could certainly live with the extra UI preference if others like it > (Dani, you want the "workspace is king" scenario exposed in the UI?) Thanks for taking a look Martin. Does anyone object to 'fix + tests 5'?
> Does anyone object to 'fix + tests 5'? Sorry, but I really don't like having this change in behavior happening for free without a clear way for users to control it. Here is an example scenario: 1) I open a file in Eclipse, edit it, save the file, and close 2) The file gets changed unexpectedly by an external process 3) I right-click the file in Eclipse and select "Commit" to CVS/Git. -> The contents that were committed are *not* the contents I last saw in the editor. Because IFile#getContents() no longer throws an exception, I committed something I didn't expect and was never informed. For users who are not expecting their files to be changed externally, this is catastrophically bad behaviour. I understand there are users who frequently using external tools, and just want eclipse to always stay in sync. In those situations the free refresh makes sense. For example: 1) I edit a file with an external editor 2) Back in eclipse, I select the file and perform "commit" -> Eclipse opens an error dialog telling me the file is out of sync. Duh, of course it is, #$@*! Eclipse, I just edited it! So I completely agree with the direction of having a mode where Eclipse always honours the file system contents and never complains. I really think it makes sense to somehow tie this behaviour to the "refresh automatically" mode that is already familiar and well documented. So it seems that leaves us with solution #3, since there seems difficult for the polling monitor adjusting its behaviour automatically. Note I haven't actually reviewed any patches - I wanted us to all agree on the direction first.
Thanks John. FYI, James' argument so far was that "external modification" is not really different than "internal modification" by misbehaving plugins / builders. And, that we have several places where plugins perform a silent #refreshLocal today so we're not really protected from the scenario that you outline anyways. From what I understood, egit might perform silent autorefresh of your file so you'd be hosed regardless of what we do for this bug. So while I agree with you in principle, I also see the situation pragmatic. As I said, I'd prefer "fix+tests 5" but I can also live with "fix+tests 3".
(In reply to comment #61) > -> The contents that were committed are *not* the contents I last saw in the > editor. Because IFile#getContents() no longer throws an exception, I committed > something I didn't expect and was never informed. For users who are not > expecting their files to be changed externally, this is catastrophically bad > behaviour. Interesting... I tried this with a few different VCSs: - CVS reports an error. If you open a compare editor first, it silently refreshes. - egit 0.11.3 silently commits your file - subclipse silently commits your file I guess it's a pretty bad if users ended up committing stuff they're unaware of. I just don't see this as a likely scenario. The only person modifying a user's files is the user. Whether that modification comes from within or outside eclipse shouldn't matter. [A bigger problem is when metadata changes outside eclipse and eclipse remains unaware. For example: changing CVS branch on the command line and committing within Eclipse will silently commit to the wrong branch and corrupt the local metadata (bug 165158 bug 73260). The tooling *should* be robust against this...] I'd assert that most users will want refresh automatically enabled most the time. So if we want team providers to be strict, then the team provider should call #getContents(false) or #isSynchronized() on the tree. > So it seems that leaves us with solution #3, since there seems difficult for > the polling monitor adjusting its behaviour automatically. Note I haven't > actually reviewed any patches - I wanted us to all agree on the direction > first. No complaints from me. I still think we should make the assumption the user knows what they're doing, and enable by default.
(In reply to comment #62) > FYI, James' argument so far was that "external modification" is not really > different than "internal modification" by misbehaving plugins / builders. There is a significant difference here. An internal modification in Eclipse is directly and immediately reflected in the UI. For example if a plugin/builder creates or deletes a file, it appears in the Navigator immediately. If it is done externally, then there is a period where what the user sees in Eclipse does not reflect reality on disk. If the user takes an action within Eclipse during that period, they might actually be operating on something different from what they expect. The risk isn't that the user "doesn't know what they are doing"... it's that if they see stale information in Eclipse, and then take an action based on that stale information, it could be damaging. Internal modifications don't leave stale information that creates this risk. > that we have several places where plugins perform a silent #refreshLocal today > so we're not really protected from the scenario that you outline anyways. From > what I understood, egit might perform silent autorefresh of your file so you'd > be hosed regardless of what we do for this bug. Typically plugins will perform refreshLocal on a resource that they know they just modified via an external process. For example I expect egit refreshes resources that it knows it just modified externally by invoking jgit. This in turn was probably initiated by a user action within the egit UI.
> So it seems that leaves us with solution #3, since there seems difficult for > the polling monitor adjusting its behaviour automatically. Note I haven't > actually reviewed any patches - I wanted us to all agree on the direction > first. Also +1 for the direction proposed with solution #3 with three objections: 1. The default should not be changed (as currently done in the patch) 2. I'm not sure the user understands the difference between 'Refresh automatically' and 'Refresh file system Automatically'. 3. From a quick look at the patch it looks like the meaning of the existing API constant 'PREF_AUTO_REFRESH' got changed.
(In reply to comment #65) > Also +1 for the direction proposed with solution #3 with three objections: > 2. I'm not sure the user understands the difference between 'Refresh > automatically' and 'Refresh file system Automatically'. > 3. From a quick look at the patch it looks like the meaning of the existing > API constant 'PREF_AUTO_REFRESH' got changed. Ok. I did this because I wanted to hold true to the JavaDoc description of the preference. PREF_AUTO_REFRESH states: "* Name of a preference for configuring whether the workspace performs auto- * refresh." This is still true after the patch. patch 3 adds an additional preference to control more expensive polling / filesystem hook refresh. If we flip it around and add a preference for async-refresh (as fix+tests 5 does) then PREF_AUTO_REFRESH no longer does auto-refresh, instead the other preference does. Note what you propose (and is in patch 5) will likely confuse API users forever more. If an API caller wants to know whether they can automatically RefreshLocal then they expect to check the PREF_AUTO_REFRESH preference, not check some new ASYNC_REFRESH preference. Note what you've suggested in comment 34 will give the user *the same* control. I guess your complaint is that auto_refresh preference no longer hooks the filesystem. fix+tests 5 will do what you want, just the UI patch needs updating for the new preference and appropriate strings. (I personally think v3 is cleaner if we're going to expose a preference, though). > 1. The default should not be changed (as currently done in the patch) Really? You're the only person to object to this. Is there no way to change your mind on this?
Created attachment 189895 [details] UI IDE patch for fix + tests 5 UI preference for fix + tests 5. Provides: [ ]Refresh Automatically [ ] Refresh files on access Ticking refresh automatically ticks both. The Refresh files on access preference controls the new behaviour.
(In reply to comment #66) > > 1. The default should not be changed (as currently done in the patch) > Really? You're the only person to object to this. > Is there no way to change your mind on this? I would like to point out as well that there are 25 votes and counting for this bug. It's pretty clear what the community wants...
So... I think we're all in line that we want the new feature in, we're just not all in line yet how to best expose it. Perhaps it helps if we collect some acceptance criteria, to see what each of us "must have" or "could live with"? From my point of view, by order of priority (most important first): - Semantics of existing API constants MUST remain unchanged. That is, the PREF_AUTO_REFRESH must remain tied to the refresh monitors, even if that makes the new constant harder to explain or the API docs hard to understand. - As a corrolary, there MUST be a new Preference slot for the new feature. - Semantics of existing UI controls SHOULD remain unchanged. I see the importance of keeping existing UI documentation correct, but on the other hand I value improvement if we can make the final UI better understandable for end users - even at the expense of changing behavior of existing UI slightly. - The new feature MAY be exposed in the UI. I'd rather not confuse users by the new setting, but I can certainly live with exposing the setting in the UI (and it seemed that John required having it in the UI). - Default values of Preferences MAY be changed. These affect "plain Open Source" users only since any product builders are expected to leverage plugin_customization.ini anyways - so it's natural to allow flipping the default setting to reflect the best we have to offer at any moment in time. So that should explain the spectrum of solutions that I'd find acceptable... it looks like "fix+tests 5" is out since John wanted an explicit UI control, and "fix+tests 3" is the likely direction with some extra polish to be done. (In reply to comment #67) > [ ]Refresh Automatically > [ ] Refresh files on access I don't like this wording in the UI since I agree with Dani that users won't understand what it means. My personal favorite for the UI is still from comment #60, involving a slight change of UI representation: [x] Refresh automatically <-> tied to NEW pref slot, default on [ ] Allow file system polling <-> tied to PREF_AUTO_REFRESH, dflt off
(In reply to comment #69) > [x] Refresh automatically <-> tied to NEW pref slot, default on > [ ] Allow file system polling <-> tied to PREF_AUTO_REFRESH, dflt off I should also mention that in case we can distinguish "polling" file system monitors from "OS optimized integrated" file system monitors (and if we can't distinguish those yet we should add API to distinguish them), I could see us tying the new preference to enabling both "refresh on file access" as well as "allow optimized refresh monitors to refresh". Since what the user wants is keeping their workspace up-to-date with the file system, regardless of how it's implemented technically. For lack of a better idea, the new preference slot could also be named PREF_AUTO_REFRESH_2 similar to the way we evolve interface API.
Thanks Martin. (In reply to comment #69) Make sense up to here. > So that should explain the spectrum of solutions that I'd find acceptable... it > looks like "fix+tests 5" is out since John wanted an explicit UI control, and > "fix+tests 3" is the likely direction with some extra polish to be done. Well both fix 3 + 5 have a core.resources preference. v3 changed the semantics of the existing refresh preference to split out heavy-weight polling. v5 added a new preference to control the new functionality. UI exists for both versions. The new UI patch for v5 meets all the requirements you've outlined, I believe. > I don't like this wording in the UI since I agree with Dani that users won't > understand what it means. That's Dani's wording ;). I have no objection to changing the UI in any way to make this easier to grok. > My personal favorite for the UI is still from comment #60, involving a slight > change of UI representation: > [x] Refresh automatically <-> tied to NEW pref slot, default on > [ ] Allow file system polling <-> tied to PREF_AUTO_REFRESH, dflt off Yes, that seems neat to me. I think this is just a name change to the preferences in the v5 UI patch...
(In reply to comment #70) > I should also mention that in case we can distinguish "polling" file system > monitors from "OS optimized integrated" file system monitors (and if we can't > distinguish those yet we should add API to distinguish them), I could see us > tying the new preference to enabling both "refresh on file access" as well as > "allow optimized refresh monitors to refresh". This seems to contradict your most important criterion in comment #69: "PREF_AUTO_REFRESH must remain tied to the refresh monitors". I could go either way on this point. My gut feeling is that PREF_AUTO_REFRESH be tied to "refresh on access" and "native refresh monitors". I.e., this preference means "We will make all reasonable efforts to keep the workspace automatically in sync with the file system". The second preference would be only related to the non-native polling monitor. That way if we ever found a way to making the polling behave well, or implemented native refresh on all file systems, that second preference could go away. For example on Windows the second preference would not appear at all because there is efficient native support for automatic refresh. On the question of what is the default value, for me this comes down to timing. With M6 next week, there is not much time to address any follow-on bugs if there are problems with the patch, or if other plugins need to make adjustments to this. My preference is either: - Release it in M6, disabled by default. We put an entry in N&N and encourage people to try it out. If all goes well we make it default in M7 - Release it immediately after M6, enabled by default. This gives us time to react and potentially disable for M7 if it causes major unforeseen problems.
(In reply to comment #72) > This seems to contradict your most important criterion in comment #69: > "PREF_AUTO_REFRESH must remain tied to the refresh monitors". Not quite. The meaning of PREF_AUTO_REFRESH up to now is "keep my workspace in sync even if it is costly". That's what we've been telling users in our docs. And, any products on top of Eclipse which enable this in their plugin_customization.ini today will want their workspace refreshed even at the cost of poling if necessary. I think that this meaning shouldn't be changed. We can, however, introduce a new PREF_AUTO_REFRESH_2 which means "keep my workspace refreshed when it's easy to do but don't use polling". We could also name it PREF_LIGHTWEIGHT_AUTO_REFRESH if people like that. Again, I don't care much about the default value, so if turning the feature off by default is your criterion for allowing it into M6, I'd rather have it off but available in M6. > I could go either way on this point. My gut feeling is that PREF_AUTO_REFRESH > be tied to "refresh on access" and "native refresh monitors". I agree that this would be the ideal case, but to me the "don't change existing semantics" argument is stronger than "make the API intuitive" so I prefer the above.
I'm away for a week. So if someone could take this on for commit to M6, that would be appreciated. AFAIK this is pretty much ready. All that may need doing is changing UI strings in the UI patch + potentially re-factor the preference name in 'fix + tests 5'. If not I guess we can bash at this some more for M7.
Created attachment 191362 [details] core.resources + UI + tests 6 (In reply to comment #70) > (In reply to comment #69) > > [x] Refresh automatically <-> tied to PREF_AUTO_REFRESH_2, default on > > [ ] Allow file system polling <-> tied to PREF_AUTO_REFRESH, dflt off Implemented in the attached patch. Equivalent of patch 5 + renamed preference + UI updated.
*** Bug 337446 has been marked as a duplicate of this bug. ***
Created attachment 191898 [details] core.resources + UI + tests 7 Polish: - Rename Preference: PREF_AUTO_REFRESH_2 -> PREF_LIGHTWEIGHT_AUTO_REFRESH - Move Preference from public ResourcesPlugin -> internal RefreshManager as we can't change API without permission.
(In reply to comment #77) > Created attachment 191898 [details] > core.resources + UI + tests 7 > > Polish: > - Rename Preference: PREF_AUTO_REFRESH_2 -> PREF_LIGHTWEIGHT_AUTO_REFRESH > - Move Preference from public ResourcesPlugin -> internal RefreshManager as we > can't change API without permission. We should split the UI and core.resources part. When we get the approval for new API, we will add the UI. I would also disable the new refresh by default to see that all tests on the server pass. I would just enable it in tests checking the lightweight refresh.
FileSystemResourceManager#autoRefreshEnabled and #isFastAutoRefreshEnabled could be renamed to #lightweightAutoRefreshEnabled and #isLightweightAutoRefreshEnabled to avoid confusion. FSRM implements deprecated Preferences.IPropertyChangeListener now. Couldn't we address it not using deprecated API? Javadoc or FSRM#asyncRefresh should mention which autorefresh it is about. I'm still looking at the code. Will let you know if I find something else, James.
On more place where doc should be fixed IFile#setContent javadoc says: * is a convenience method returning an open input stream. It's equivalent to: * <pre> * getContents(auto_refresh enabled); * </pre> I would clarify what auto_refresh it is about.
Created attachment 191906 [details] core.resources + tests 8 Address Szymon's comments. UI unchanged from previous patch.
Having changed to non-deprecated preferences the new tests seem to fail when running the whole testsuite. They pass when run individually. I'm not sure what's wrong yet...
FSRM#asyncRefresh(IResource target) I would update the javadoc to "Asynchronously auto-refresh the requested resource if {@link RefreshManager#PREF_LIGHTWEIGHT_AUTO_REFRESH} is enabled." instead of "Asynchronously auto-refresh the requested resource if Auto-Refresh is enabled." I think that FSRM#isSynchronized javadoc is bad. It says: "Any discovered out-of-sync resources are scheduled to be brought back in sync." It should be: "Any discovered out-of-sync resources are scheduled to be brought back in sync, if {@link RefreshManager#PREF_LIGHTWEIGHT_AUTO_REFRESH} is enabled."
Created attachment 191910 [details] core.resources + tests 9 - Change JavaDoc on core.resources internal methods as per Szymon's comment - Move back to deprecated Plugin Preferences API. The preference change listener / preference node at InstanceScope disappears randomly while running the full set of automated tests...
When the patch is in HEAD and builds are fine, we have to raise separate bugs for the API change and the UI.
Committed to HEAD. API changes requested in bug 340977, UI proposed in bug 340978
Marking it fixed.
*** Bug 303492 has been marked as a duplicate of this bug. ***
There is a slight performance regression (5-8%) in JDT/Core search tests due to this fix. I see the regression only on windows.
(In reply to comment #89) > There is a slight performance regression (5-8%) in JDT/Core search tests due to > this fix. I see the regression only on windows. Could you point me at the results you've got? The latest nightly with performance results (against 3.6): http://download.eclipse.org/eclipse/downloads/drops/N20110430-2000/performance/org.eclipse.jdt.core.php?fp_type=0 doesn't seem to show this.
Look for FullSourceWorkspaceSearchTests#testSearchBinaryMethod()in that page. Though it shows green, it shows -8% for the windows machine. The graph doesn't show much but if you could look at the performance numbers directly by clicking on "Raw data and stats", that could help. The numbers can be directly accessed at http://download.eclipse.org/eclipse/downloads/drops/N20110430-2000/performance/epwin2/raw/Scenario196.html. You could clearly see that the regression has started between I20110329-0800 and N20110324-2000.
Thanks for pointing this out. (In reply to comment #91) > Look for FullSourceWorkspaceSearchTests#testSearchBinaryMethod()in that page. > Though it shows green, it shows -8% for the windows machine. I think this is because org.eclipse.jdt.internal.core.util.Util, explicitly does getContents(true) i.e. it never reports out-of-sync (and has been this way since 2003...). This behaviour is different to that in platform text search. According to Dani this behaviour is wrong: bug 337446 comment 1 ; bug 337446 comment 3 This fix made the code path for #getContents(false) the same as #getContents(true). In both cases sync is checked. Previously getContents(true) didn't check the sync state. Backtrace: FileSystemResourceManager.read(IFile, boolean, IProgressMonitor) line: 754 File.getContents(boolean) line: 289 Util.getResourceContentsAsCharArray(IFile, String) line: 1186 Util.getResourceContentsAsCharArray(IFile) line: 1160 JavaSearchDocument.getCharContents() line: 53 PossibleMatch.getContents() line: 76 ImportMatchLocatorParser(Parser).parse(ICompilationUnit, CompilationResult, int, int) line: 9614 ImportMatchLocatorParser(Parser).parse(ICompilationUnit, CompilationResult) line: 9586 ImportMatchLocatorParser(Parser).dietParse(ICompilationUnit, CompilationResult) line: 8178 MatchLocator.parseAndBuildBindings(PossibleMatch, boolean) line: 1591 MatchLocator.locateMatches(JavaProject, PossibleMatch[], int, int) line: 1019 MatchLocator.locateMatches(JavaProject, PossibleMatchSet, int) line: 1124 MatchLocator.locateMatches(SearchDocument[]) line: 1256
> I think this is because org.eclipse.jdt.internal.core.util.Util, explicitly > does getContents(true) i.e. it never reports out-of-sync (and has been this > way since 2003...). > This behaviour is different to that in platform text search. According to > Dani this behaviour is wrong: bug 337446 comment 1 ; bug 337446 comment 3 I was speaking about the default behavior of the SDK/UI. That doesn't mean one cannot write tests that ignore the resource's sync state by asking for the content directly. > This fix made the code path for #getContents(false) the same as > #getContents(true). In both cases sync is checked. Previously > getContents(true) > didn't check the sync state. Why do we check the sync state when 'true' is passed? The idea of that argument is that one can ignore the sync state.
(In reply to comment #93) > I was speaking about the default behavior of the SDK/UI. That doesn't mean one > cannot write tests that ignore the resource's sync state by asking for the > content directly. It's not the test, it's jdt.core Util class, which is used by the search search. You can see this in the normal platform: FileSystemResourceManager.read(IFile, boolean, IProgressMonitor) line: 754 File.getContents(boolean) line: 289 Util.getResourceContentsAsCharArray(IFile, String) line: 1186 Util.getResourceContentsAsCharArray(IFile) line: 1160 JavaSearchDocument.getCharContents() line: 53 PossibleMatch.getContents() line: 76 ImportMatchLocatorParser(Parser).parse(ICompilationUnit, CompilationResult, int, int) line: 9716 ImportMatchLocatorParser(Parser).parse(ICompilationUnit, CompilationResult) line: 9688 ImportMatchLocatorParser(Parser).dietParse(ICompilationUnit, CompilationResult) line: 8280 MatchLocator.parseAndBuildBindings(PossibleMatch, boolean) line: 1591 MatchLocator.locateMatches(JavaProject, PossibleMatch[], int, int) line: 1019 MatchLocator.locateMatches(JavaProject, PossibleMatchSet, int) line: 1124 MatchLocator.locateMatches(SearchDocument[]) line: 1256 JavaSearchParticipant.locateMatches(SearchDocument[], SearchPattern, IJavaSearchScope, SearchRequestor, IProgressMonitor) line: 94 BasicSearchEngine.findMatches(SearchPattern, SearchParticipant[], IJavaSearchScope, SearchRequestor, IProgressMonitor) line: 231 BasicSearchEngine.search(SearchPattern, SearchParticipant[], IJavaSearchScope, SearchRequestor, IProgressMonitor) line: 515 SearchEngine.search(SearchPattern, SearchParticipant[], IJavaSearchScope, SearchRequestor, IProgressMonitor) line: 584 JavaSearchQuery.run(IProgressMonitor) line: 144 InternalSearchUI$InternalSearchJob.run(IProgressMonitor) line: 91 Worker.run() line: 54 > Why do we check the sync state when 'true' is passed? The idea of that argument > is that one can ignore the sync state. Well we don't need to if !lightweightrefresh. However with lightweight refresh on (which I'm hoping becomes the default in 3.8) this API will check sync in both cases.
Created attachment 194837 [details] fix for the performance regression
(In reply to comment #95) This patch fixes the performance regression - Thanks.
(In reply to comment #95) > Created attachment 194837 [details] > fix for the performance regression Hi James, Do you plan to release this patch?
This is a one-liner (whitespace excluded). John: ok to commit?
As it for 3.7RC1 I've opened bug 344954
*** Bug 268797 has been marked as a duplicate of this bug. ***
*** Bug 363240 has been marked as a duplicate of this bug. ***
