Bug 301226 - Improve error handling/reporting in case of unusable configuration area
Summary: Improve error handling/reporting in case of unusable configuration area
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Framework (show other bugs)
Version: 3.5   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 3.6 M6   Edit
Assignee: Andrew Niefer CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 287339 (view as bug list)
Depends on: 303071
Blocks:
  Show dependency tree
 
Reported: 2010-01-29 04:18 EST by Anton Leherbauer CLA
Modified: 2010-04-14 15:48 EDT (History)
4 users (show)

See Also:


Attachments
Proposed fix (6.33 KB, patch)
2010-01-29 07:44 EST, Anton Leherbauer CLA
no flags Details | Diff
Patch with suggested changes (4.28 KB, patch)
2010-02-18 09:05 EST, Anton Leherbauer CLA
aniefer: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anton Leherbauer CLA 2010-01-29 04:18:00 EST
Build id: I20090611-1540

When I start eclipse with the -configuration <config-area> option and the specified directory is read-only or otherwise not usable, I get an exception in the shell (see below), but this exception does not really explain the problem and eclipse even launches, but it is unusable and throws exceptions all over the place.

Exception logged on the shell:

!MESSAGE Error reading configuration: /local/toni/.eclipse/configuration/org.eclipse.osgi/.manager/.fileTableLock (No such file or directory)
!STACK 0
java.io.FileNotFoundException: /local/toni/.eclipse/configuration/org.eclipse.osgi/.manager/.fileTableLock (No such file or directory)
        at java.io.RandomAccessFile.open(Native Method)
        at java.io.RandomAccessFile.<init>(RandomAccessFile.java:212)
        at org.eclipse.core.runtime.internal.adaptor.Locker_JavaNio.lock(Locker_JavaNio.java:32)
        at org.eclipse.osgi.storagemanager.StorageManager.lock(StorageManager.java:388)
        at org.eclipse.osgi.storagemanager.StorageManager.open(StorageManager.java:686)
        at org.eclipse.osgi.internal.baseadaptor.BaseStorage.initFileManager(BaseStorage.java:213)
        at org.eclipse.osgi.internal.baseadaptor.BaseStorage.initialize(BaseStorage.java:147)
        at org.eclipse.osgi.baseadaptor.BaseAdaptor.initializeStorage(BaseAdaptor.java:121)
[...]

I suggest to add an early configuration area check to the launcher and clearly identify and report back what's wrong with the location instead of just logging exceptions and keep running.
Comment 1 Anton Leherbauer CLA 2010-01-29 07:44:50 EST
Created attachment 157612 [details]
Proposed fix

Attached patch adds a check for the configuration directory for following error conditions:

- the directory cannot be created
- the directory is not writable
- the directory does not support the java.nio file locking method (if no other method is specified)

In case of an error, exitcode and exitdata describing the error are reported back to the launcher executable which will show a dialog with the error.
Comment 2 Martin Oberhuber CLA 2010-01-29 09:52:38 EST
This bug is related to 
- bug 59780 (improper error reporting when workspace cannot be locked), and
- bug 230384 (improper error reporting when parent of config area is not
             writable, because p2 artifacts are generated in the parent dir)

I think that this is an important usability problem to address. 

End users of our product on top of Eclipse have often run into situations where they accidentally had trouble with file systems being read-only or not lockable, and they could not understand the errors being reported. In fact we have been patching Eclipse in our products in the past to address this, and one motivation for contributing this patch now is that we would prefer consuming an unmodified Eclipse.
Comment 3 Martin Oberhuber CLA 2010-02-15 13:46:53 EST
Just wondering, when somebody could do an initial triage of this?

Attached patch is tested to work fine, and just changes
    src/org/eclipse/equinox/launcher/Main.java
adding error checks and reporting to address important usability concerns.
Comment 4 Thomas Watson CLA 2010-02-15 14:45:29 EST
Also see bug 301431.

It should be possible to launch eclipse from a completely read-only configuration by using osgi.configuration.area.readonly property.  We should not be checking if we canWrite or if isLockingAllowed if osgi.configuration.area.readonly property is defined.

Also the ability to write in the configuration/ folder may not be good enough.  There have been several issues reported where the user may have write access to the configuration folder but not necessarily to every sub-folder under the configuration area.  This can also cause the IOExceptions when attempting to lock the file configuration/org.eclipse.osgi/.manager/.fileTableLock

It is unfortunate that we have to do the nio check this early.  I think the fact that we could not open the StorageManager in the framework but we still allow the framework to launch is a bug that we should consider fixing instead of forcing the launcher to attempt a lock.
Comment 5 Anton Leherbauer CLA 2010-02-17 09:46:38 EST
(In reply to comment #4)
> Also see bug 301431.
> 
> It should be possible to launch eclipse from a completely read-only
> configuration by using osgi.configuration.area.readonly property.  We should
> not be checking if we canWrite or if isLockingAllowed if
> osgi.configuration.area.readonly property is defined.

Sure, we can add that check.

> Also the ability to write in the configuration/ folder may not be good enough. 
> There have been several issues reported where the user may have write access to
> the configuration folder but not necessarily to every sub-folder under the
> configuration area.  This can also cause the IOExceptions when attempting to
> lock the file configuration/org.eclipse.osgi/.manager/.fileTableLock

I think that's good enough for a basic sanity test.  On the other hand, if the framework did handle this case better, the check might be obsolete.

> It is unfortunate that we have to do the nio check this early.  I think the
> fact that we could not open the StorageManager in the framework but we still
> allow the framework to launch is a bug that we should consider fixing instead
> of forcing the launcher to attempt a lock.

I agree, improving the framework is preferable.
Comment 6 Thomas Watson CLA 2010-02-17 10:44:24 EST
(In reply to comment #5)
> > It is unfortunate that we have to do the nio check this early.  I think the
> > fact that we could not open the StorageManager in the framework but we still
> > allow the framework to launch is a bug that we should consider fixing instead
> > of forcing the launcher to attempt a lock.
> 
> I agree, improving the framework is preferable.

I opened bug 303071 to address the failed configuration lock issue.  Please remove the lock check in the next patch.
Comment 7 Anton Leherbauer CLA 2010-02-18 09:05:59 EST
Created attachment 159429 [details]
Patch with suggested changes

- Added test for property "osgi.configuration.area.readOnly" 
  No check is done if set.
- Removed java.nio locking check
Comment 8 Thomas Watson CLA 2010-02-19 17:17:06 EST
I tested the patch and it looks fine.  I am not sure about the need or way you add newlines to the message though.  Andrew is this needed.  Will the text just wrap if we do not put the newlines?
Comment 9 Andrew Niefer CLA 2010-03-03 14:39:35 EST
The message box does get automatically wrapped, we think it looks better without the newlines.

Also, it is not enough to have the .readonly property defined, it must equal "true".

I released a version without the newlines and that uses Boolean.valueOf to check that the property is "true".
Comment 10 Martin Oberhuber CLA 2010-03-03 15:54:55 EST
Thanks Andrew! That's great.
I assume that Anton deserves an iplog+ on the patch, he's a committer on CDT but not on RT or Platform.
Comment 11 Andrew Niefer CLA 2010-03-03 15:58:19 EST
Comment on attachment 159429 [details]
Patch with suggested changes

(In reply to comment #10)
> Thanks Andrew! That's great.
> I assume that Anton deserves an iplog+ on the patch, he's a committer on CDT
> but not on RT or Platform.

Sorry, yes it gets a +iplog
Comment 12 Thomas Watson CLA 2010-04-14 15:48:42 EDT
*** Bug 287339 has been marked as a duplicate of this bug. ***