Created attachment 91703 [details]
stacktrace

I need full IDE log because I added some logging to diagnose this exception. Without that I am not able to decide if it is still old properties issue or another one. Please attach it.

Maven customizer tries to modify read only file. It should be handled better. I will attach call stack from IDE log.

  Product Version         = NetBeans IDE 6.8 Beta (Build 200910212001) (#4e7504cf593f)
  Operating System        = Linux version 2.6.18-5-686-bigmem running on i386
  Java; VM; Vendor        = 1.6.0_11; Java HotSpot(TM) Client VM 11.0-b16; Sun Microsystems Inc.
  Runtime                 = Java(TM) SE Runtime Environment 1.6.0_11-b03

From reporter:
The Maven project whose source was monitored by a SCM tool (Accurev) had read only permissions on it's directories and when a rename action was attempted, NB threw an exception I remember.

Created attachment 91900 [details]
Exception call stacks from IDE log

This bug already has 5 duplicates 
see http://statistics.netbeans.org/exceptions/detail.do?id=162931

Created attachment 97648 [details]
stacktrace

Reassigning to default owner.

attempt to rename a project which is locked by VCS will result in an exception.

(In reply to comment #8)
> attempt to rename a project which is locked by VCS will result in an exception.

Yes, it probably will. That does not permit AbstractModel.endTransaction to throw an undocumented unchecked exception.

Hm, this does not seem to have a clean solution. 

I may document the fact that endTransaction() throws ISE, but no code actually handles that state (at this moment). So although formally the ISE will be at least documented, users won't get a meaningful message in the "file locked" situations.

Looking at the tx related methods - startTransaction() returns status code that (almost) nobody checks, does not even attempt to lock the file to be edited.

(In reply to comment #10)
> startTransaction() returns status code that (almost) nobody checks

BTW after 836a8d22b1b3 this should be detected by FindBugs.

Created attachment 118800 [details]
Proposed changes

The endTransaction() is just documenting that it may throw an exception. In the reality, it already throws it under the described conditions, so the change only documents the de-facto state.

No objections or comments, going to push the change tomorrow.

The patch by itself does not solve the actual bug. Every call to this method now has to be updated to catch ISE and handle it appropriately.

Dependent bugs filed against client code in the main hg repository with proposed patches.

Thanks for working on the client patches. It is too bad the original API design did not consider the possibility that endTransaction could fail this way, and throw IOException or something.

Is it really necessary to throw ISE in Model.endTransaction() ?
May be Model.endTransaction() implementation should not throw ISE at all ?
I don't like unchecked exception throwing.

I would change the code:
catch (Exception ex) {
            throw new IllegalArgumentException("Exception during flush:);
}

to catching exactly exception related to RO document and handle it separately 
( just log with some Level ( INFO ? )).

All other exceptions could be re-thrown as IAE or ISE.

That will eliminate the API change and necessity  to change all client code.

The API change is not (primarily) a change from IAE to ISE, but a change that documents the ISE being thrown - requires future clients to handle it or deliberately ignore it. ISE is thrown from other parts of endTransaction() execution (e.g. XDMAccess), so having the code to throw IAE+ISE (present state) does not make any sense, that's why I changed the ISE from catch-all handler in AbstractModel, too. You're right that the exc. handling in AbstractModel should be improved - catch(Exception) is usually bad.

If I left IAE + ISE being thrown, clients would be tempted to use the poor design of overly broad exception handlers (e.g. catch Exception, Throwable) even in their code.

BTW - throwing IAE from a 0-arg method is strange :)

re. not throwing at all: IMHO, the decision whether to continue with modification after failed endTransaction() should be up to the client - e.g. imagine that the client calls endTransaction() on a RO document (which does not obviously propagate the changes to the disk) and then deletes the original content from another document. Data loss (= P1 defect) may occur. But some clients may continue without any troubles, or just by displaying a note in the status bar - it depends on what the client's code does depending on successful XAM modification.

In order to decide, the client must get the information that the call had failed - a status code or, better, an exception.

(In reply to comment #18)
> The API change is not (primarily) a change from IAE to ISE, but a change that
> documents the ISE being thrown - requires future clients to handle it or
> deliberately ignore it. ISE is thrown from other parts of endTransaction()
> execution (e.g. XDMAccess), so having the code to throw IAE+ISE (present state)
> does not make any sense, that's why I changed the ISE from catch-all handler in
> AbstractModel, too. You're right that the exc. handling in AbstractModel should
> be improved - catch(Exception) is usually bad.
> 
> If I left IAE + ISE being thrown, clients would be tempted to use the poor
> design of overly broad exception handlers (e.g. catch Exception, Throwable)
> even in their code.
No problem with ISE instead of IAE.
The question is exception necessity at all.
> 
> BTW - throwing IAE from a 0-arg method is strange :)
Agree. But once again : I don't care about special type of exception. But
exception requirement.
> 
> re. not throwing at all: IMHO, the decision whether to continue with
> modification after failed endTransaction() should be up to the client - e.g.
> imagine that the client calls endTransaction() on a RO document (which does not
> obviously propagate the changes to the disk) and then deletes the original
> content from another document. Data loss (= P1 defect) may occur. But some
> clients may continue without any troubles, or just by displaying a note in the
> status bar - it depends on what the client's code does depending on successful
> XAM modification.
> 
> In order to decide, the client must get the information that the call had
> failed - a status code or, better, an exception.
Not any transaction involvement is WRITE model modification.
Transaction is required for any ATOMIC model access.
So one can block model via transaction to get atomic exclusive access to model 
in the thread. In this case no modification will be done at all. 
I don't know exactly the implementation code of endTransaction(). Will it 
throw an exception if nothing has been changed in model ?
Probably endTransaction() is not the correct method for throwing exception 
if modification is prohibited .

If modification is prohibited from the start, startTransaction() should return false - return value is ignored in all code I've seen (which is a mistake, obviously). Some conditions however cannot be easily determined, e.g. VCS-locked file, which is unlocked at the time of write ... or the unlock may fail. But even though the caller checks that write is enabled at startTransaction(), the condition (eg. readonly flag) may change DURING transaction.

Javadoc for XAM Model says:
"A transaction >> must << be acquired during a mutation .. Only a single transaction at a time is supported". Nothing in javadoc promises atomicity - readers not using transaction can see even partial model mutations.

The endTransaction() calls XDM's flush() on the model and could fail for different reasons from its inception:
* the document model not being 'stable', e.g. is currently not well-formed. Even parsing could report 'not stable' state :)
* someone locked the source file
I think I could find some more, but the above two are out of control of the mutating thread, and both can lead to the XDM unable to apply consistently changes to the underlying document.

I don't think that we can silently trash information about endTransaction() actual failure to update the base document in the log, but I can be convinced by solid arguments.

(In reply to comment #20)

> 
> Javadoc for XAM Model says:
> "A transaction >> must << be acquired during a mutation .. Only a single
> transaction at a time is supported". Nothing in javadoc promises atomicity -
> readers not using transaction can see even partial model mutations.
That's true: without transaction one can see partial model mutation.
When model is locked exclusively via transaction then any access ( read or 
write ) is atomic.
> 
> The endTransaction() calls XDM's flush() on the model and could fail for
> different reasons from its inception:
> * the document model not being 'stable', e.g. is currently not well-formed.
> Even parsing could report 'not stable' state :)
This case is separate and has no relation to transaction.
Transaction is model side operation but "parsing" is result of "sync" and
"stable" state is determined via state of model. This state should be and
is actually checked in the clients. 
> * someone locked the source file
> I think I could find some more, but the above two are out of control of the
> mutating thread, and both can lead to the XDM unable to apply consistently
> changes to the underlying document.
> 
> I don't think that we can silently trash information about endTransaction()
> actual failure to update the base document in the log, but I can be convinced
> by solid arguments.

OK, let's use the suggested signature of endTransaction().
Thanks for explanations.

Integrated into 'main-golden', will be available in build *201205190400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/f071920feb7c
User: Svata Dedic <sdedic@netbeans.org>
Log: #177636: documented IllegalStateException thrown from endTransaction()