Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 323833

Summary:

Files in file cache are marked as immutable and are not deleted on restart

Product:

[Eclipse Project] Platform

Reporter:

Eduard Bartsch <eduard.bartsch>

Component:

Resources

Assignee:

Szymon Brandys <Szymon.Brandys>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

Dmitry_Karasik, Szymon.Brandys

Version:

3.6

Flags:

Szymon.Brandys: review+

Target Milestone:

3.7 M4

Hardware:

Macintosh

OS:

Mac OS X - Carbon (unsup.)

Whiteboard:

Attachments:

Description	Flags
Patch proposal	none
Updated patch that reflects Szymon's review	none
Updated patch that clears the immutable flag at startup	Szymon.Brandys: iplog+
Fixed patch	none

Description Eduard Bartsch

2010-08-27 10:26:46 EDT

unfortunately, the fix for bug 297228 creates a side effect (at least) on Mac with regard to the method FileStore.toLocalFile(EFS.CACHE, null) when applied to a read-only file store. 

A cached file that is created by the above method in .metadata/.plugins/org.eclipse.core.filesystem/filecache/ is marked as IMMUTABLE that prevents it's deletion at Eclipse/JavaVM shutdown. The VM fails to delete the file albeit the file.deleteOnExit() is requested.

It also leads to an exception on the next Eclipse startup in FileCache.cleanOldCache() because the file in the old cache can not be deleted using java.io.File.delete().

I think that the FileCache implementation should take that files in file cache are not marked as immutable but only as read-only.

Alternatively (or additionally), the cache cleanup should be more robust and also delete immutable files.

An exception in FileCache.cleanOldCache() is propagated to the callers of FileStore.toLocalFile that is called in many places (for example for local workspace history) so that the situation is disturbing.

Comment 1 Eduard Bartsch

2010-08-31 10:12:00 EDT

Created attachment 177838 [details]
Patch proposal

I have created a patch proposal that removes the IMMUTABLE flag when a read-only file is written into cache.

Comment 2 Eduard Bartsch

2010-09-10 06:03:59 EDT

We would need the fix backported to 3.6.2 as well.

Comment 3 Szymon Brandys

2010-09-14 11:46:12 EDT

(In reply to comment #1)
IFileStore#toLocalFile javadoc says

* While the implementation of this method may use caching to return the
* same result for multiple calls to this method, it is guaranteed that the 
* returned file will reflect the state of this file store at the time of this call.

so resetting the IMMUTABLE flag in FileCache#cache would be a breaking change. Maybe we could improve FileCache#cleanOldCache, so it could clean IMMUTABLE files at startup.

Comment 4 Eduard Bartsch

2010-09-15 07:31:11 EDT

Created attachment 178914 [details]
Updated patch that reflects Szymon's review

If we take the Javadoc literally, the current behaviour of FileStore.toLocalFile() doesn't match the Javadoc because it adds the EFS.ATTRIBUTE_IMMUTABLE to the target store albeit the source store has on the read-only attribute set. For the LocalFile, the read-only and the immutable flags are always set/cleared together. But for other file stores it is not the case.

I have updated the patch so that it now clears the immutable flag only when the source store is read-only and no immutable flag is set on it. Now, the state of the cached file matches exactly the state of the source store. And the update patch still has the advantage that cached files are deleted on VM shutdown.

The updated patch will solve the issue we face in our scenario. The general problem with caching of read-only LocalFiles still remains. But the fix for that is not limited for the FileCache class because FileCache.cleanOldCache() reuses the method LocalFile.delete(). I am not sure that LocalFile.delete() should be able to delete immutable files. May be, a user has decided to make some files immutable on purpose. 

An alternative that is local to the FileCache class would be to implement FileCache.cleanOldCache() directly without reusing LocalFile.delete(). I can propose a patch that duplicates the source code form the LocalFile class and tweaks it to fix this bug but I don't like copy/paste style of programming. 

Another alternative would be to have an additional method on LocalFile class that is able to delete immutable files.

Comment 5 Szymon Brandys

2010-10-06 05:32:50 EDT

Ed, please update the patch according to our recent agreement. I think it was to clear the cache at startup.

Comment 6 Eduard Bartsch

2010-10-13 08:22:18 EDT

Created attachment 180759 [details]
Updated patch that clears the immutable flag at startup

As we agreed, here is the patch that clears the flag on the next startup.

Comment 7 Szymon Brandys

2010-11-09 12:46:39 EST

Created attachment 182736 [details]
Fixed patch

Comment 8 Szymon Brandys

2010-11-10 03:37:32 EST

Fixed in HEAD.