Community
Participate
Working Groups
This is a problem found in RSE 3.0.3 1. Create a linux connection 2. Create a file with the name abc.c and write some string in it 3. Create anohter file with the name ABC.c 4. Open the file ABC.c in editor and it contains unreadable characters instead of being empty 5. The file name on the title bar of the editor is abc.c instead of ABC.c 6. Put another string in the opened file and save it and close it 7. Open the file abc.c again and the content in the editor is the one in file ABC.c 8. Open the file ABC.c while abc.c is opened and nothing happens From now on, it doesn't matter which file you open, you always get the file name abc.c and content of ABC.c. The content of the file abc.c can't be showed again. And all the changes go to ABC.c. There was only one cached file in RSE cache and it's abc.c with the content of ABC.c. When log on the Linux system, both files are fine and contain different contents.
I renamed the file ABC.c to test.c and then tried to open abc.c. I still couldn't open it. The download job kept running and didn't finish.
When I canceled the download job, the following exception was thrown on the host side. Exception in thread "Thread-219" java.lang.NullPointerException at org.eclipse.dstore.core.model.DataElement.initialize(DataElement.java :1601) at org.eclipse.dstore.core.model.DataElement.initialize(DataElement.java :1548) at org.eclipse.dstore.core.model.DataElement.reInit(DataElement.java:256 ) at org.eclipse.dstore.core.model.DataStore.createObject(DataStore.java:1 332) at org.eclipse.dstore.core.model.DataStore.createObject(DataStore.java:1 262) at org.eclipse.dstore.core.model.DataStore.createObject(DataStore.java:1 232) at org.eclipse.dstore.core.model.DataStore.createObject(DataStore.java:1 197) at org.eclipse.rse.internal.dstore.universal.miners.filesystem.Universal DownloadHandler.handleDownload(UniversalDownloadHandler.java:268) at org.eclipse.rse.internal.dstore.universal.miners.filesystem.Universal DownloadHandler.run(UniversalDownloadHandler.java:65)
I can reproduce this in RSE 3.0.3 but it seems okay to me in RSE 3.1. Samuel, could you try to reproduce this using RSE 3.1?
It's better in 3.1 that the file in the editor actually switches when different file is opened. But the change made in the file is not saved when you just open another file to switch. It seems that you have to close one and open another to save the changes.
Created attachment 135699 [details] patch for dealing with cached file of different case Samuel, could you try out this patch?
Dave, Sorry that the problem as in comment #4 still exists.
(In reply to comment #6) > Dave, > Sorry that the problem as in comment #4 still exists. > So you're not getting prompted to save when opening the file with the other case?
Created attachment 135804 [details] updated patch I just realized that I missed one of the files when creating the patch. Can you try this?
Here is my scenario. 1. Open file abc.c 2. Open file ABC.c and make a change 3. Save file ABC.c 4. Close file ABC.c and open it again 5. The change made in step 2 was lost
(In reply to comment #9) > Here is my scenario. > 1. Open file abc.c > 2. Open file ABC.c and make a change > 3. Save file ABC.c > 4. Close file ABC.c and open it again > 5. The change made in step 2 was lost > Did you try this with the updated patch?
Hi Dave, The second patch looks better. But our tester still found the following issues. 1. Simply right-clicking on TEST.txt will close test.txt if you're currently editing it. If you have changed pending, you will be prompted to save your changes, however. Here's where it gets bad. If you click cancel, you will be notified of a save conflict between the remote and local files. If you choose to overwrtite the remote file with the local version, the remote file will be replaced with a blank file. -On occasion, reproducing this bug prevented either test.txt or TEST.txt from being opened until eclipse was restarted. They could still be opened with the Open With submenu. - This error is also reproducible with two files of the same name in case-differentiated-only folders. 2. If you have text.txt and abc.txt both open, and rename abc.txt to TEST.txt, then double-click the newly renamed test.txt, you will . If you save in one edit window, you will not be notified of cache conflicts in the other, though both windows will overwrite the same file. This generates save conflicts but does not obliterate the remote file.
Two more comments from our tester. I have noticed that if you rename a file or folder to a case-differentiated-only name, the originals are not deleted. This is going to create orphaned cache files. If you create a new file pointing to an orphan, however, the cache does download a new remote copy so you won't get the old file popping up in the edit window. Still, in terms of cache bloat, the orphaning glitch should be brought to the RSE team's attention. Renaming a file or folder where there is NOT a case-differentiated-only name does not create this issue. Conversely, if you have two files: test.txt and TEST.txt, and you've currently cached test.txt, renaming TEST.txt to any arbitrary filename will also rename the cached file. While this doesn't matter - as it seems they redownload every file from the remote list when you open them. However, it does obliterate the current cached file.
Created attachment 136015 [details] enhanced patch with handling for menu-based open as well For issue #1, could you try this new patch? For the renaming issues, I'd prefer to work with a separate defect - could you open that separately?
(In reply to comment #13) > Created an attachment (id=136015) [details] > enhanced patch with handling for menu-based open as well > > For issue #1, could you try this new patch? For the renaming issues, I'd > prefer to work with a separate defect - could you open that separately? > Additional problem found in this patch: 1. Open test.txt for editing. Add some changes. (Note double-clicking TEST.txt will prompt you to save changes. Handles properly from what I can see, both cached and remote files are updated properly.) 2. Right click -> open TEST.txt. Note editor contents are not updated and no prompts to save changes or warnings are created. However, user is now editing TEST.txt. Saving the file will overwrite the remote TEST.txt. The first time I attempted this, no save conflict warning appeared. However, following attempts to reproduce this error generated a save confict warning.
Additionally, it should be noted that opting to replace the local content with the remote copy, rather than overwriting the remote file, will not change the content of the browser window as one would expect. It will, however, not harm the remote file. Closing and reopening TEST.txt will fix this problem. Otherwise, you will receive a "file changed" warning after about a minute.
Created attachment 136326 [details] updated patch including additional open paths I missed a couple methods of opening. Could try with this patch?
With the latest patch, I found a new issue. 1. With test.txt open for editing, synchronize the cache. 2. Click on the editing screen. You will be warned that the local contents have changed. Click yes. 3. Editor will now contain the content of TEST.txt
(In reply to comment #17) > With the latest patch, I found a new issue. > > 1. With test.txt open for editing, synchronize the cache. > 2. Click on the editing screen. You will be warned that the local contents have > changed. Click yes. > 3. Editor will now contain the content of TEST.txt > I think, in step one, you need to edit the file too, right?
It occurred when the file was unchanged, for me. I didn't test with pending changes.
(In reply to comment #19) > It occurred when the file was unchanged, for me. I didn't test with pending > changes. > You can reproduce that consistently? I've tried several times but I'm only able to reproduce this when the editor is dirty.
Reproduces consistently with a clean editor window for me. Before opening test.txt, try opening and closing TEST.txt and see if that helps.
(In reply to comment #21) > Reproduces consistently with a clean editor window for me. Before opening > test.txt, try opening and closing TEST.txt and see if that helps. > Okay, I think I'd like to deal with this synchronize cache issue in bug 276534 since that also deals with the Synchronize Cache action.
I've committed the patch to cvs. Additional issues with the Synchronize Cache will be dealt with in bug 276534.
This did not go into RC1 because RC1 was built on the 19th, and I'm quite uncertain whether it is appropriate to put it into RC2. The fix is quite large, and from reading the code I do not understand all implications. It looks like it's restricted to certain error conditions related to SystemEditableRemoteFile only, but I could not tell for sure. The code looks like quickly hacked together, there seem to be a lot of copy-and-paste style duplications. Why do we try to address this so late in the cycle? It's been known for years that the way how RSE caches remote files cannot work properly when I'm on windows and the remote is UNIX. What we really need is a different mapping of remote file names to local file names, when the local FS is not case sensitive. See bug 160100 comment 6, and bug 160100 comment 13. The mapping scheme I find most usable is described in bug 160100 comment 9. Trying to fix the SystemEditableRemoteFile without also fixing the mapping of names to the underlying file system seems problematic. What happens with API calls such as IRemoteFile file_a = subsys.getRemoteFileObject("/foo/a"); IRemoteFile file_A = subsys.getRemoteFileObject("/foo/A"); would that work correctly? Or would it pick up a wrong cached file? I'm not quite reopening this yet since the fix is in HEAD, but I'd like to see some justification and discussion around this. Also note that we are in RC endgame so you'll need another committer's approval before this can be released.
(In reply to comment #24) > This did not go into RC1 because RC1 was built on the 19th, and I'm quite > uncertain whether it is appropriate to put it into RC2. The fix is quite large, > and from reading the code I do not understand all implications. It looks like > it's restricted to certain error conditions related to SystemEditableRemoteFile > only, but I could not tell for sure. > > The code looks like quickly hacked together, there seem to be a lot of > copy-and-paste style duplications. > > Why do we try to address this so late in the cycle? It's been known for years > that the way how RSE caches remote files cannot work properly when I'm on > windows and the remote is UNIX. What we really need is a different mapping of > remote file names to local file names, when the local FS is not case sensitive. > See bug 160100 comment 6, and bug 160100 comment 13. The mapping scheme I find > most usable is described in bug 160100 comment 9. > > Trying to fix the SystemEditableRemoteFile without also fixing the mapping of > names to the underlying file system seems problematic. Yes, fixing the mapping scheme will be the ultimate fix for this kind of problem but I don't think we can do that right now without a significant amount of work and possibly API changes. I felt that it was important to address this now (with something shorter-term) since there is the potential for lost data. > What happens with API > calls such as > > IRemoteFile file_a = subsys.getRemoteFileObject("/foo/a"); > IRemoteFile file_A = subsys.getRemoteFileObject("/foo/A"); > > would that work correctly? Or would it pick up a wrong cached file? The getRemoteFileObject() calls don't have an issue with this since they're not actually dealing with cached files - just IRemoteFiles. > > I'm not quite reopening this yet since the fix is in HEAD, but I'd like to see > some justification and discussion around this. Also note that we are in RC > endgame so you'll need another committer's approval before this can be > released. > I'm not sure whether you're okay with the justification. If so, I'll add a committer to review.
Justification of the change is a different thing than reviewing correctness of the change anyways. Before asking somebody to review the large number of changes, is there a chance to reduce the number of changes (by re-using code rather than copy / paste) ? Also, an explanation of the hi-level ideas behind your changes would be helpful.
(In reply to comment #26) > Justification of the change is a different thing than reviewing correctness of > the change anyways. > > Before asking somebody to review the large number of changes, is there a chance > to reduce the number of changes (by re-using code rather than copy / paste) ? > I duplicated certain pieces of code (namely the getEditableRemoteObject() code) because I didn't want to introduce new API but I suppose I could introduce internal API for that, reducing the duplication. > Also, an explanation of the hi-level ideas behind your changes would be > helpful. > The workings behind the getEditableRemoteObject() code are as follows: -we first determine whether there is an existing remote editable object via the IFile properties -if there is, then we need to check whether the associated IFile is of a different case from the one we want -if the case is different, then we're going to need to close this editor before opening a new one with the file we want to open and if there are changes to the currently open editor, we need to save them and upload first -regardless of whether the editor is open or not, the old temp file of different case is to be deleted
Ok, the description sounds fair enough. I understand that it will only fix the dbl click / edit file action. Under what circumstances will RSE still report the wrong file from the cache even with your fix, and what would the worst case behavior be? Do you see any risks associated with your change? If you could refactor your code to use "internal" API I think this would be better than code duplication. In terms of the mechanics for getting these changes in, I can either revert HEAD to what's currently released (and you attach patches), or we are optimistic and you commit these changes to HEAD (and we review in CVS by means of the "Team > Compare with > Released" function from the releng.tools plugin. Use the method that you prefer. Thanks!
Created attachment 137523 [details] patch to put put getEditableRemoteObject() code in common place Here's a patch with the common getEditableRemoteObject() code moved to SystemRemoteEditManager. Note that SystemViewRemoteFileAdapter.getEditableRemoteObject() still exists on it's own because the use there is different from the edit actions. For the adapter case, the editor handling is taken care of after the call.
(In reply to comment #28) > Ok, the description sounds fair enough. > > I understand that it will only fix the dbl click / edit file action. Under what > circumstances will RSE still report the wrong file from the cache even with > your fix, and what would the worst case behavior be? > UniversalFileTransferUtility.getTempFileFor() will return an existing resource of a different case if it exists. It used to return an IFile using the specified case, which is misleading when a file already exists in a different case. > Do you see any risks associated with your change? The difference for the edit actions is that we're checking for an existing SystemEditableRemoteFile now whereas before we would just blindly construct a new SystemEditableRemoteFile. I think this is safer than what we were doing before. > > If you could refactor your code to use "internal" API I think this would be > better than code duplication. I created a new patch with refactored code. > > In terms of the mechanics for getting these changes in, I can either revert > HEAD to what's currently released (and you attach patches), or we are > optimistic and you commit these changes to HEAD (and we review in CVS by means > of the "Team > Compare with > Released" function from the releng.tools plugin. > Use the method that you prefer. Thanks! > I'd prefer to make changes on top of those already in HEAD.
I've committed the changes to cvs.
Reviewing the new patch, it looks OK. Only thing I notice is that the code in SystemViewRemoteFileAdapter should better re-use some code from SystemRemoteEditManager rather than duplicating it. To me it seems like all special code paths can only ever be entered when a local SystemEditableRemoteFile is associated with a remote absolute path different than what the file gets found for, so there is a case discrepancy. That is, the current situation is getting improved. I'm approving this for RC4, but I still think the real solution must come from bug 160100. Can you check whether re-using SystemRemoteEditManager code in SystemViewRemoteFileAdapter is actually possible.
I'd also like to mention that the main risk I see with this approach is that an interactive operation (dbl clicking on remote FOO.c) leads to a lot of synchronous operations on the UI thread (closing editor of local foo.c, potentially uploading changes)! I'd like to see some testing with some local open editors (dirty or not) when a case variant of the same file gets opened. Does the upload work OK? What if a local file is modified but the remote is read-only so "doUploadImmediately" fails? Are we at risk of loss of data? Releasing for RC4 but please have a look at these concerns.
