IFile.write: IllegalArgumentException

[jukzi]

Replacing IFile.setContents with IFile.write in org.eclipse.jdt.internal.core.Buffer.save(IProgressMonitor, boolean) causes a regression:

java.lang.IllegalArgumentException: Attempted to beginRule: P/P, does not match outer scope rule: L/P/Test.java
    at org.eclipse.core.runtime.Assert.isLegal(Assert.java:68)
    at org.eclipse.core.internal.jobs.ThreadJob.illegalPush(ThreadJob.java:144)
    at org.eclipse.core.internal.jobs.ThreadJob.push(ThreadJob.java:416)
    at org.eclipse.core.internal.jobs.ImplicitJobs.begin(ImplicitJobs.java:66)
    at org.eclipse.core.internal.jobs.JobManager.beginRule(JobManager.java:343)
    at org.eclipse.core.internal.resources.WorkManager.checkIn(WorkManager.java:124)
    at org.eclipse.core.internal.resources.Workspace.prepareOperation(Workspace.java:2402)
    at org.eclipse.core.internal.resources.File.create(File.java:174)
    at org.eclipse.core.resources.IFile.write(IFile.java:441)
    at org.eclipse.jdt.internal.core.Buffer.save(Buffer.java:401)
    at org.eclipse.jdt.internal.core.CommitWorkingCopyOperation.executeOperation(CommitWorkingCopyOperation.java:115)
    at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:739)
    at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2452)
    at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2477)
    at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:809)
    at org.eclipse.jdt.internal.core.CompilationUnit.commitWorkingCopy(CompilationUnit.java:398)
    at org.eclipse.jdt.core.tests.model.EncodingTests.test031(EncodingTests.java:599)

I will investigate. If someone has an idea: please share

[iloveeclipse]

org.eclipse.jdt.internal.core.CommitWorkingCopyOperation.getSchedulingRule() uses modify rule (== current file) for already existing files, but new API in org.eclipse.core.resources.IFile.write(byte[], boolean, boolean, boolean, IProgressMonitor) unconditionally calls org.eclipse.core.internal.resources.File.create(byte[], int, IProgressMonitor) that needs also parent rule to be taken as is supposed to modify the parent state too (by creating a new child)

[mickaelistria]

As a file doesn't pre-exist, the create operation uses the parent project scheduling rule. Since write calls create it does use this parent project scheduling rule. If the operation in process already uses the file scheduling rule (because it knows it will only modify this file), then trying to get a "bigger" scheduling rule for the operation will fail. As create can now either create a new file or modify it, it's rule could probably be changed to final ISchedulingRule rule = exists() ? workspace.getRuleFactory().modifyRule(this) : workspace.getRuleFactory().createRule(this);, or maybe just clearly have create first line or so be if (exists() && (flags & REPLACE) != 0) { setContents(); } to clearly split stuff.

[mickaelistria]

The clearer documentation about scheduling rule is probably https://www.eclipse.org/articles/Article-Concurrency/jobs-api.html , but there are some other info available in the help such as https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Fguide%2FresAdv_concurrency.htm or https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Fguide%2FresAdv_batching.htm (the later is exactly describing what JDT is doing here: grouping several workspace changes to this file into a single operation, with the narrowest possible scheduling rule in order to allow more concurrent processing)

[jukzi]

I am not used to that rules ... do you think it should be handled in JDT or does it need to be fixed in platform?

[mickaelistria]

It should be changed in Platform so that editing an existing file should always use the modifyResource(file) rule. As suggested earlier, create should probably be reorganized to just call the good old setContents when file already exists, so it would behave exactly the same, not even changing the scheduling rule.

[iloveeclipse]

I am not used to that rules ... do you think it should be handled in JDT or does it need to be fixed in platform?

Platform. "Write" can't take "Create" rule, as the "Create" scope is bigger.

[jukzi]

setContents when file already exists, so it would behave exactly the same, not even changing the scheduling rule.

That would essentially revert the only review suggestion, i.e. also remove the potential usage of a CREATE REPLACE flag?

[mickaelistria]

Yes, maybe it's simpler to just remove the usage of REPLACE (which doesn't bring much per se) and just make write just delegate to setContents or create depending on exists()

[jukzi]

thanks @mickaelistria and @iloveeclipse for your valuable input on this