Community
Participate
Working Groups
IJobFunction interface and Job.create(String name, IJobFunction function) method allow for creation of jobs using lambdas. Unfortunately, the signature of IJobFunction.run method requires few lines of boiler-plate code in the lambda body. Consider for example a method that does asynchronous project refresh: void refreshAsync(IProject project) { Job.create("Refreshing " + project.getName(), (IProgressMonitor monitor) -> { try { project.refreshLocal(IResource.DEPTH_INFINITE, monitor); } catch (CoreException e) { return e.getStatus(); } return Status.OK_STATUS; }).schedule(); } It would be nicer to be able to write this method as: void refreshAsync(IProject project) { Job.create("Refreshing " + project.getName(), (IProgressMonitor monitor) -> project.refreshLocal(IResource.DEPTH_INFINITE, monitor) ).schedule(); } To achieve that we can define a new interface and a corresponding Job.create function: public interface IThrowingJobFunction { public void run(IProgressMonitor monitor) throws CoreException; } public static Job create(String name, final IThrowingJobFunction function) { return new Job(name) { @Override protected IStatus run(IProgressMonitor monitor) { try { function.run(monitor); } catch (CoreException e) { return e.getStatus(); } return Status.OK_STATUS; } }; } Given frequent use of system jobs it would be nice to have dedicated factory methods for them: public static Job createSystem(@Nullable String name, final IThrowingJobFunction function) { Job job = create(name == null ? "" : name, function); job.setSystem(true); return job; } public static Job createSystem(final IThrowingJobFunction function) { Job job = create("", function); job.setSystem(true); return job; } If you can think of a better name than IThrowingJobFunction please come forward.
Stefan Xenos pointed out offline that IThrowingJobFunction is identical to IWorkspaceRunnable and therefore redundant.
I like the idea of simplifying the usage of Job.create but AFAICS Job.create is not used in the Eclipse platform code base. Before making it easier to use, I suggest we switch some existing code to the usage of this new method. Created Bug 474273 for this.
Lars, shouldn't we implement the new method first and then adopt it? Or are you suggesting that we implement the new method in the same commit that adopts it? Adopting the old, inconvenient, convenience method and then immediately replacing it with the one described here seems a bit silly.
(In reply to Stefan Xenos from comment #3) > Lars, shouldn't we implement the new method first and then adopt it? Or are > you suggesting that we implement the new method in the same commit that > adopts it? > > Adopting the old, inconvenient, convenience method and then immediately > replacing it with the one described here seems a bit silly. The Job.create method is currently not used. Making it easier to use should be based on some usage. Just search for the usage of the static method Job.create. It is currently not used at all.
The belongsTo method seems to used quite intensive in the existing code base.
Yuck. I thought default methods were supposed to save us from this proliferation of interfaces, but I can't think of a way off-hand to bridge this behavior using the same IJobFunction interface. IWorkspaceRunnable doesn't help because it is in core.resources which core.jobs does not depend on.
New Gerrit change created: https://git.eclipse.org/r/53272
New Gerrit change created: https://git.eclipse.org/r/53273
(In reply to John Arthorne from comment #6) Yuck, indeed. I ended up introducing ICoreRunnable interface that is defined in the same package as IProgressMonitor and CoreException. I wish we could replace IWorkspaceRunnable with it, but the best we can possibly do is to make IWorkspaceRunnable extend ICoreRunnable.
John, can you commit this so we can start using it?
+Thomas Watson. Thomas, could I trouble you to take a look at the attached changes?
(In reply to Stefan Xenos from comment #11) > +Thomas Watson. > > Thomas, could I trouble you to take a look at the attached changes? The addition of ICoreRunnable seems fine, but why must it be defined in the org.eclipse.core.runtime package instead of in the org.eclipse.core.job package? Will this interface be used outside of the jobs API?
(In reply to Thomas Watson from comment #12) > The addition of ICoreRunnable seems fine, but why must it be defined in the > org.eclipse.core.runtime package instead of in the org.eclipse.core.job > package? Will this interface be used outside of the jobs API? ICoreRunnable can be used outside of the jobs API. We definitely don't want to repeat the mistake with IWorkspaceRunnable, which is functionally equivalent to ICoreRunnable but is defined in a wrong plugin. See comment #6.
Gerrit change https://git.eclipse.org/r/53272 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=8a8f71942a1034d52acef423a973eee8415f576b
(In reply to Sergey Prigogin from comment #13) > (In reply to Thomas Watson from comment #12) > > The addition of ICoreRunnable seems fine, but why must it be defined in the > > org.eclipse.core.runtime package instead of in the org.eclipse.core.job > > package? Will this interface be used outside of the jobs API? > > ICoreRunnable can be used outside of the jobs API. We definitely don't want > to repeat the mistake with IWorkspaceRunnable, which is functionally > equivalent to ICoreRunnable but is defined in a wrong plugin. See comment #6. I merged into master the new ICoreRunnable interface. Someone else needs to have a look at changes for jobs. They seem fine to me, but I am not well experienced in using or developing the jobs implementation.
Gerrit change https://git.eclipse.org/r/53273 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=39f7e813a28381b5d52012ff9fb89ad7e48433ea
Merged. Thanks guys...
Horray!
(In reply to Stefan Xenos from comment #18) > Horray! Can you add an N&N entry for the whole change? Would be nice if other Eclipse projects get information about it.
New Gerrit change created: https://git.eclipse.org/r/55604
This new requirement in IJobFunction#run(IProgressMonitor) is a breaking change for existing callers that should be mentioned in the porting guide: * It is the caller's responsibility to call {@link IProgressMonitor#done()} * after this method returns or throws an exception. A bit further up in the Javadoc, there's this: * Jobs can optionally finish their execution asynchronously (in another thread) by * returning a result status of {@link Job#ASYNC_FINISH}. Jobs that finish * asynchronously <b>must</b> specify the execution thread by calling * <code>setThread</code>, and must indicate when they are finished by calling * the method <code>done</code>. The reference to the "method <code>done</code>" is confusing, since IJobFunction doesn't offer such a method. If the intention is that the job function calls Job#done(IStatus), then this reference should be a link to that method.
Guys, you need to install API Tools: https://wiki.eclipse.org/Version_Numbering#API_Baseline_in_API_Tools That would have told you that org.eclipse.core.jobs needs to be bumped to version 3.8.0. There a more required bundle version changes that are a bit harder to see, because we don't have tooling to detect them: - org.eclipse.core.resources consumes the new ICoreRunnable, so it need to raise the lower bound of the dependency on org.eclipse.core.runtime (which reexports org.eclipse.equinox.common) - org.eclipse.core.runtime needs to raise the lower bound for required bundle org.eclipse.equinox.common to 3.8.0. Which implies raising o.e.c.runtime to 3.12.0. I've fixed the problems in the eclipse.platform.runtime repo with http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=5ae1c8307a07b3333a73be575fb2867a75af1292 I'll open a Gerrit for the resources part. But to avoid breaking people who don't have both repos in source, that should only be pushed after the above change has landed in an I-build.
Gerrit change https://git.eclipse.org/r/55604 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=ef20642a0222da47282b124f0e1d8b1d07e21788
New Gerrit change created: https://git.eclipse.org/r/55666
New Gerrit change created: https://git.eclipse.org/r/56111
Gerrit change https://git.eclipse.org/r/56111 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=0ce9c7b59416f1224c5ad5faac9188440078b284
Is this bug still open?
There is still one outstanding change in Gerrit, but the new Job creation methods are in place.
Open issues to be resolved in M3: - comment 21 (breaking API change; should maybe be reverted -- didn't investigate) - https://git.eclipse.org/r/55666
New Gerrit change created: https://git.eclipse.org/r/56187
(In reply to Markus Keller from comment #21) > This new requirement in IJobFunction#run(IProgressMonitor) is a breaking > change for existing callers that should be mentioned in the porting guide: > > * It is the caller's responsibility to call {@link > IProgressMonitor#done()} > * after this method returns or throws an exception. A change from an underspecified contract to a properly specified one is not breaking, but I agree that it makes sense to mention it in the porting guide. Could you please point me to the porting guide so that I could add a note there. > A bit further up in the Javadoc, there's this: > > * Jobs can optionally finish their execution asynchronously (in another > thread) by > * returning a result status of {@link Job#ASYNC_FINISH}. Jobs that finish > * asynchronously <b>must</b> specify the execution thread by calling > * <code>setThread</code>, and must indicate when they are finished by > calling > * the method <code>done</code>. > > The reference to the "method <code>done</code>" is confusing, since > IJobFunction doesn't offer such a method. If the intention is that the job > function calls Job#done(IStatus), then this reference should be a link to > that method. Will be addressed by https://git.eclipse.org/r/#/c/56187/
Gerrit change https://git.eclipse.org/r/56187 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=620c87e546993c078ee29787c7d8fa1a6757c1b3
The change in IJobFunction#run(IProgressMonitor) is breaking for third-party callers. You changed Job#create(String, IJobFunction) to call "monitor.done()", but there may be other callers of that method which didn't have to do that before, but now they have to. Even more breaking changes happened in IWorkspaceRunnable#run(IProgressMonitor), which used to have this precondition: * @param monitor a progress monitor, or <code>null</code> if progress * reporting and cancellation are not desired Since bug 476142, it inherits ICoreRunnable#run(IProgressMonitor), which says: * @param monitor the monitor to be used for reporting progress and * responding to cancellation. The monitor is never {@code null}. * It is the caller's responsibility to call {@link IProgressMonitor#done()} * after this method returns or throws an exception. => progress monitor cannot be null any more => caller has to call done() Your assumption that IWorkspaceRunnable#run(..) is only called by Workspace#run(..) is not valid. These changes are not absolutely necessary and they need to be reverted. The IWorkspace#run(..) APIs can specify that - the monitor passed to the action is never null and - this method calls done() on that monitor The porting guide would have been here: http://git.eclipse.org/c/platform/eclipse.platform.common.git/tree/bundles/org.eclipse.platform.doc.isv/porting
(In reply to Markus Keller from comment #33) Since there will be many more implementations of ICoreRunnable than the places that call its run method, it's pretty important that the contract of ICoreRunnable puts convenience of the implementors over convenience of the callers of ICoreRunnable.run. It is also pretty important that the contract is fully specified. Since the contract of IWorkspaceRunnable did not satisfy either of these requirements, the only viable solution is to disconnect IWorkspaceRunnable from ICoreRunnable while preserving IWorkspace.run methods that accept ICoreRunnable. What is your opinion?
New Gerrit change created: https://git.eclipse.org/r/56850
New Gerrit change created: https://git.eclipse.org/r/56851
For existing implementations of IWorkspaceRunnable#run(..), the additional requirements on @param monitor are not an issue (non-null monitor is fine, and calling done() twice is also fine). These changes are only problematic for old callers of that method. I think we can keep IWorkspaceRunnable as a subtype of ICoreRunnable if we just restore the API of IWorkspaceRunnable#run(..). The last caveat is that implementations of IWorkspaceRunnable#run(..) may throw an OperationCanceledException. I don't think it's a problem if we documents this in ICoreRunnable#run(..). Existing callers of that method already do handle OperationCanceledException. Gerrits to finish this up: https://git.eclipse.org/r/56850 https://git.eclipse.org/r/56851 (In reply to Markus Keller from comment #33) > The change in IJobFunction#run(IProgressMonitor) is breaking for third-party > callers. You changed Job#create(String, IJobFunction) to call > "monitor.done()", but there may be other callers of that method which didn't > have to do that before, but now they have to. This is unlikely to be a problem in practice, and I'm OK with accepting this change. But it needs to be mentioned in the porting guide in 4.6/incompatibilities.html. The addition of ICoreRunnable should be added in 4.6/recommended.html.
(In reply to Markus Keller from comment #37) I've adjusted and clarified the ICoreRunnable's contract in the latest patch set in https://git.eclipse.org/r/56850 to allow for null monitor. This change doesn't put additional burden on the implementers of the interface as long as they use SubMonitor. This change should make any additional comments in IWorkspaceRunnable unnecessary.
(In reply to Sergey Prigogin from comment #38) > This change should make any additional comments > in IWorkspaceRunnable unnecessary. The spec for cancellation and the nullness of the monitor are fine in https://git.eclipse.org/r/#/c/56850/3 . What remains is this new requirement: [..] It is the caller's responsibility to * call {@link IProgressMonitor#done()} after this method returns or throws * an exception. Callers of IWorkspaceRunnable didn't have to call #done() before. https://git.eclipse.org/r/#/c/56851/2 reduces the IWorkspaceRunnable#run(...) Javadoc to keep this API.
(In reply to Markus Keller from comment #39) > Callers of IWorkspaceRunnable didn't have to call #done() before. The contract for calling done() was not specified, but it doesn't mean that nobody had responsibility for calling done. Wokspace.run methods did call done() method themselves creating a precedent for other callers of IWorkspaceRunnable.run. If some implementations didn't follow the IWorkspaceRunnable.run example, they will continue to work, but would technically be in violation of the new fully specified calling contract. It's preferable that these implementations are brought in compliance with the ICoreRunnable contract, rather than creating an exemption for them in IWorkspaceRunnable.
> The contract for calling done() was not specified, but it doesn't mean > that nobody had responsibility for calling done I agree. The old version (where the contract on done was unspecified) was a problem, and the new version documents the best practices. However: perhaps I could suggest an alternative wording. Really, the caller is only required to call done if it chose to use a progress monitor that requires the use of done... and there's not many of those left (I think SubProgressMonitor is the only one left). Instead of this: "It is the caller's responsibility to call {@link IProgressMonitor#done()} after this method returns or throws an exception." We could say this: "The receiver is not responsible for calling {@link IProgressMonitor#done()} on the given monitor, and the caller must not rely upon the fact that {@link IProgressMonitor#done()} was called." ...so we invert the phrasing and state that the receiver is not required to do anything with done. This should be completely compatible with the old contract which also - through omission - didn't impose any requirements on the receiver. It also reflects reality better. Most likely, the caller is either using a top-level monitor which has a special case for termination or it is using SubMonitor which doesn't require done... so probably - in practice - nobody will call done or needs to call done.
(In reply to Sergey Prigogin from comment #40) > The contract for calling done() was not specified, but it doesn't mean that > nobody had responsibility for calling done. Nope, the contract of IProgressMonitor already defines the normal usage: * The <code>IProgressMonitor</code> interface is implemented * by objects that monitor the progress of an activity; the methods * in this interface are invoked by code that performs the activity. * [...] When the task is eventually completed, a * <code>done()</code> notification is reported. IWorkspaceRunnable#run(..) didn't miss a special contract for done(); it just went with the defaults. Deprecating SubProgressMonitor doesn't give you a free pass to break APIs. (In reply to Stefan Xenos from comment #41) > "The receiver is not responsible for calling {@link IProgressMonitor#done()} > on the given monitor, and the caller must not rely upon the fact that {@link > IProgressMonitor#done()} was called." Sounds good for ICoreRunnable#run(..). But IWorkspaceRunnable#run(...) still can't redefine the protocol.
"Notifies that the work is done; that is, either the main task is completed or the user canceled it. This method may be called more than once (implementations should be prepared to handle this case)." > IWorkspaceRunnable#run(..) didn't miss a special contract for done(); it > just went with the defaults. There is nothing in the quote you included that suggests whether done should be invoked by the caller of a method or the receiver. It just says that done needs to be invoked by *someone*, and I don't think anyone ever suggested breaking that contract. If I said something you interpreted otherwise, I either misspoke or you misunderstood - because that wasn't my intent. What did I say that you interpreted as violating the contract on IProgressMonitor.done()? > Deprecating SubProgressMonitor doesn't give you a free pass to break APIs. Accusing me of wanting to break APIs doesn't add anything material to the discussion of whether or not this is actually a breaking change - and if I understand you correctly, I believe you may have intended to give offense. Let's work together to make decisions that are best for Eclipse rather than take sides and throw accusations.
> Deprecating SubProgressMonitor doesn't give you a free pass to break APIs. You seem to think I'm arguing that we should break the contract on IProgressMonitor#done(). Let me explain: I'm trying to argue for the same thing sergey was -- that we should document where the call to done() needs to go rather than leaving it undocumented as it was previously. My suggestion, above, was to say the same thing as sergey originally suggested, but to phrase it in the negative so as not to give the impression to callers that they have any new responsibilities to call done() on top of the responsibilities they already inherit from their progress monitor.
Re: comment 43, comment 44 I just re-read that and it sounded a bit obnoxious. Sorry, Markus. Wish I could edit and re-send that. :-(
(In reply to Stefan Xenos from comment #43) > There is nothing in the quote you included that suggests whether done should > be invoked by the caller of a method or the receiver. Yes there is, right after the ';' in the first paragraph of IProgressMonitor: * [...]; the methods * in this interface are invoked by code that performs the activity. In IWorkspaceRunnable, the code that performs the activity is the implementation of the run(..) method. (In reply to Stefan Xenos from comment #45) No hard feelings here. After all, I started the direct accusations. I realize I sometimes use harsher words than I should, and I have no problems when others do the same. Better get loud and bring the discussion forward than not state your point. I really thought you meant that SubMonitor's "BEST PRACTISES" approach is the only solution we support now, and I'm glad to see that impression was wrong. I think it would make sense to have IProgressMonitor refer to SubMonitor and mention the "new rules" in methods whose default contract often gets adjusted in new APIs. Unfortunately, I'll be away for 1.5 weeks now, and I'll not be able to work on this.
In response to a comment by Sebastian Zarnekow at https://git.eclipse.org/r/#/c/56850/3: > Maybe I don't fully understand the implications of CoreException(CANCEL_STATUS) vs > OperationCanceledException, but I see this code in Workspace.run(..) > } catch (OperationCanceledException e) { > getWorkManager().operationCanceled(); > throw e; > } > shouldn't there be something equivalent for CoreException with a CANCEL_STATUS if both > exceptions are considered to be equivalent? I agree that the current behavior is inconsistent and has to be corrected. The possibility of throwing CoreException with STATUS_CANCEL has always existed and the only logical interpretation of such an exception is to assume that the operation was canceled. Filed bug 478774 to track that discrepancy.
Gerrit change https://git.eclipse.org/r/56850 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=cfd2db785b81221deeaaa8e3850a89e5415f33c6
Re: Comment 47 IStatus has other uses besides being attached to a CoreException thrown from a long-running operation (Logging, populating message dialogs, etc.). IMO, we should deprecate the use of CoreException for job cancellation, and focus on OperationCanceledException. This doesn't mean we necessarily need to deprecate the CANCELLED status code itself... just its use as a marker for job cancellation. Anyway, I've submitted the latest patch to equinox so we can mark this as fixed. I've filed bug 479670 for a follow-up issue.
Marking as fixed.
Gerrit change https://git.eclipse.org/r/55666 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.resources.git/commit/?id=011cbdab62fdd1d81ee9c5e118b66eeac95858b9
https://git.eclipse.org/r/56851 still needs to be pushed. As explained at length in previous comments, the current Javadoc would require all callers of IWorkspaceRunnable#run(IProgressMonitor) to call IProgressMonitor#done().
(In reply to Markus Keller from comment #52) > https://git.eclipse.org/r/56851 still needs to be pushed. As explained at > length in previous comments, the current Javadoc would require all callers > of IWorkspaceRunnable#run(IProgressMonitor) to call IProgressMonitor#done(). Sergey, can you please take a look at that change? Thanks.
Gerrit change https://git.eclipse.org/r/56851 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.resources.git/commit/?id=a103a81e0fdc99d89cd533655e742515846b68f1
