Bug 578871 - add ISchedulableOperation #30
Conversation
to prevent UI freeze during UNDO
|
Shouldn't that be put into org.eclipse.core.commands bundle to other "Operations" in org.eclipse.core.commands.operations? Job's runtime API does not need this operation. |
yea, but operations has no dependency to runtime. so it's not possible without adding additional dependencies. |
There was a problem hiding this comment.
Looking on eclipse-platform/eclipse.platform.ui#18, I think this code really belongs to org.eclipse.core.commands.operations
It makes no sense that you on the one hand request to not add dependencies and on ask to do so in the next review. |
|
@jukzi : what makes sense and what not is exact the question that needs to be discussed in a review. So if a reviewer asks something, probably that makes sense for him, at least. You proposing to add new API and I simply questioning if the API should be added somewhere else. This is usual during reviews, that people discuss things. I see that org.eclipse.core.commands doesn't require runtime bundle, so my first proposal to put new API there will not work. WDYT? |
workbench does not work either because UndoableOperation2ChangeAdapter does not require a workbench |
|
@iloveeclipse There is a subtle difference between a question and a request. Please reconsider your review or suggest a working alternative. Currently your vote blocks merging without adding any value. |
@jukzi : such comments are not OK, please consider to change the way you discuss things in public reviews. Now to the actual change: API is there for years, so addition of API should be made carefully, and so I think it could wait few more hours. May be someone else has a proposal or could provide other feedback. |
I've made a proposal in eclipse-platform/eclipse.platform.ui#8 , which I believe would resolve this issue and many more of that kind. However I won't be able to work on it for some time. If someone wants to give it a try, that would be welcome. |
bundles/org.eclipse.core.jobs/src/org/eclipse/core/runtime/jobs/ISchedulableOperation.java
Outdated
Show resolved
Hide resolved
|
It's totally fine to wait for others to review. I just want you to understand that it is offending to read that there are "changes requested" when they are not. |
This is github way to tell you there was a -1 comment in terms of gerrit. |
It's as well offending on github as on gerrit to not reconsidering a negative review after it was solved. |
|
@jukzi please try to avoid turning every comment into a passive-aggressive form of blaming people who are just doing their best as well, and consider just kindly asking. A comment such as "I think I covered all requested changes but the review is still -1, what do you think is missing?" is much more pleasant and ultimately more efficient. |
|
I think I covered all requested changes but the review is still -1, what do you think is missing? |
bundles/org.eclipse.core.jobs/src/org/eclipse/core/runtime/jobs/ISchedulableOperation.java
Show resolved
Hide resolved
|
I think I covered all requested changes but the review is still -1, what do you think is missing? |
|
Hint for the future: don't use "merge", use "rebase" for your commits. This way the reviewer has a chance to review the change in one commit squashed locally and not the merge commit somewhere in the middle and few other commits flying around. |
i don't find a rebase button in github. i only see "refresh and merge" in the branch. |
| package org.eclipse.core.runtime.jobs; | ||
|
|
||
| /** | ||
| * An interface to mark an Operation that needs an {@link ISchedulingRule}. |
There was a problem hiding this comment.
Here and below: please use "operation", not "Operation" because it is a word, not an API name.
| public interface ISchedulableOperation { | ||
| /** | ||
| * @return an {@link ISchedulingRule} that the operation will acquire in the | ||
| * current thread - if any. To make sure the operation can proceed |
There was a problem hiding this comment.
The new javadoc is way better compared to the old one.
Still I struggle with this: To make sure the operation can proceed without waiting for a blocking thread ... caller should call beginRule()
The problem is - this is the opposite what the API says, because beginRule() API says If the rule conflicts with another rule currently running in another thread, this method blocks.
What you actually mean is: to make sure operation can interrupt autobuild if used in context of ... but that is too much of a unrelated context for this API.
Here is my attempt to fix that (you can copy to clipboard and use Team->Apply patch in Eclipse):
diff --git a/bundles/org.eclipse.core.jobs/src/org/eclipse/core/runtime/jobs/ISchedulableOperation.java b/bundles/org.eclipse.core.jobs/src/org/eclipse/core/runtime/jobs/ISchedulableOperation.java
index 432ce62..db672d5 100644
--- a/bundles/org.eclipse.core.jobs/src/org/eclipse/core/runtime/jobs/ISchedulableOperation.java
+++ b/bundles/org.eclipse.core.jobs/src/org/eclipse/core/runtime/jobs/ISchedulableOperation.java
@@ -15,7 +15,8 @@
+import org.eclipse.core.runtime.IProgressMonitor;
+
/**
- * An interface to mark an Operation that needs an {@link ISchedulingRule}.
+ * An interface to mark an operation that needs an {@link ISchedulingRule}.
*
* @since 3.13
- *
*/
@@ -23,12 +24,14 @@
/**
+ * Gives the caller a hint if this operation may require a rule to proceed. The
+ * caller should call
+ * {@link IJobManager#beginRule(ISchedulingRule, IProgressMonitor)} before the
+ * operation and {@link IJobManager#endRule(ISchedulingRule)} after if the rule
+ * is specified.
+ *
* @return an {@link ISchedulingRule} that the operation will acquire in the
- * current thread - if any. To make sure the operation can proceed
- * without waiting for a blocking thread a caller should call
- * {@link IJobManager#beginRule(ISchedulingRule, org.eclipse.core.runtime.IProgressMonitor)}
- * before the operation and {@link IJobManager#endRule(ISchedulingRule)}
- * after. Returns {@code null} if no rule needed - in that case the
- * caller should not call neither <code>beginRule</code> nor
- * <code>endRule</code>. As this method returns only a hint the
+ * current thread - if any. Returns {@code null} if no rule needed - in
+ * that case the caller should not call neither <code>beginRule</code>
+ * nor <code>endRule</code>. As this method returns only a hint the
* operation can not assume that the caller already acquired the rule.
- * The Operation still has to acquire it - which will lead to a nested
+ * The operation still has to acquire it - which will lead to a nested
* rule.
There was a problem hiding this comment.
The new javadoc is way better compared to the old one. Still I struggle with this:
To make sure the operation can proceed without waiting for a blocking thread ... caller should call beginRule()The problem is - this is the opposite what the API says, because beginRule() API says
If the rule conflicts with another rule currently running in another thread, this method blocks.
i dont't see the problem. the trick here is that when beginRule() has been called in the same thread before the operation, then the beginRule in the operation is just nested and guaranteed to be wait free.
There was a problem hiding this comment.
i dont't see the problem
But I see it.
the trick here is that when beginRule() has been called in the same thread before the operation, then the beginRule in the operation is just nested and guaranteed to be wait free.
This is not correct.
"wait free" can only happen if there is no conflicting rule set in some other running job: If the rule conflicts with another rule currently running in another thread, this method blocks. This is same as for lock.aquire(). Sure, if current thread already acquired the lock, it is wait free.
Here we don't know in which thread what rule is used, so we can't assume that we are running the code with the rule already started and no one else is running with that rule.
I would recommend to use the proposed javadoc or provide another version if you like.
There was a problem hiding this comment.
i still don't get the point, let's figure out if we are talking about the same:
I state that:
// Caller:
beginRule(myRule,null); // 1. may block until conflicting rule ends
{ // called Operation:
beginRule(myRule,null); // 2. guaranteed wait free, because same thread
endRule(myRule);
}
endRule(myRule);
and that is what this API is for - to give the Operation the wait free beginRule (at 2.).
And the usage will then even put additional goodies, like interrupt autobuild and cancel dialog (at 1.)
There was a problem hiding this comment.
and that is what this API is for
Jörg, you seem to believe that the code here is only for your specific case - it is not, because it is public API. We don't know how API will be used once we release that and we can't assume it will be used only a case of nested call, at least here it is very generic one.
It is not called IOperationForNestedRuleBeginOnly.
If you want something only for case 2 in your example, this is not right API and actually doesn't belong to this bundle at all because it is tailored for a very specific use case. This is the reason I originally won't place this code here at all.
So if we provide generic API in jobs bundle that claims to use beginRule() to avoid waits it is just wrong message. beginRule() is used to acquire a lock, and that is a potentially blocking operation. Therefore it is confusing to provide javadoc that says the opposite.
There was a problem hiding this comment.
ok, i still can not imaging how to abuse the API but it's not important. i updated the javadoc
There was a problem hiding this comment.
ok, i still can not imaging how to abuse the API
Just assume you are 3rd party guy and have no clue about the current issue. You read the docs on job API, sees this class javadoc and say: cool, all what I need to avoid waiting is to call beginRule() at some time. You will do that and that will cause waiting and even deadlock your code! This is not cool, you are upset and Eclipse API is stupid.
I meant Eclipse. In Eclipse, if your PR branch and head of the origin are diverged, commit all your changes on PR branch, switch to "master", pull from origin to "master" branch and rebase your PR branch on master. |
yea, but before i could rebase to the upstream repo i need to refresh the upstream into my fork which results in merge :-( |
I've been wondering the same thing! It wold be nice to have a remote that connects to the unforked repo so that when you have master checked out and do a pull, it comes from the main repo. I don't want to have to keep my fork in sync with the main repo because I have to do that manually so I only want my fork when I'm working on contributing something, which is on and off. There must be a simpler way using two remotes so that I'm not switching URIs... |
|
Guys, you can have as many remotes as you want. I still have gerrit for example for jdt, because lot of patches are only there.
The way I'm working is:
|
|
Yes, I know that, but I don't know how to use them effectively. I hate to admit that out loud because I should really just figure it out. Sorry... It's also probably important to set rebase in the right place (re: the comment about a merge happening):
|
that flow seems to make sense. |
This is flexible, depending on what you want to have as result and which rights you have. If you plan only fetch from origin, you can leave "configure push" which might use different url for same repo. In most cases you are fine configuring fetch only, the push config will be derived from that automatically. |
|
I tried this approach out today contributing to PDE and it works way better than what's currently documented in the guidelines. The part I hate most is the manual syncing of my fork, but that's not relevant with this approach. One just goes back to master when done. This type of documentation, focused on using EGit in the IDE, is exactly what we need. It's also great you explain about how to do a rebase as well! Thanks! |
to prevent UI freeze during UNDO