-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent NPE after applying the refactoring session #7458
Conversation
Signed-off-by: Vladyslav Zhukovskyi <[email protected]>
ci-build |
ci-test |
Signed-off-by: Vladyslav Zhukovskyi <[email protected]>
Build # 4332 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4332/ to view the results. |
ci-build |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4333/ |
Signed-off-by: Vladyslav Zhukovskyi <[email protected]>
ci-test build report: |
ci-test |
ci-test build report: |
@@ -1171,45 +1170,38 @@ private Resource newResourceFrom(final ItemReference reference) { | |||
} | |||
|
|||
protected Promise<ResourceDelta[]> synchronize(final ResourceDelta[] deltas) { | |||
List<Promise<Void>> promisesToResolve = new ArrayList<>(deltas.length); | |||
Promise<Void> chain = promises.resolve(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not follow the logic here.
Promise<Void> chain = promises.resolve(null);
then chain = chain.thenPromise(something)
;
What you want to solve here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll explain. If we'll have the code like this:
new Promise(function(resolve, reject) {
setTimeout(() => resolve(1), 1000); // (*)
}).then(function(result) { // (**)
alert(result); // 1
return result * 2;
}).then(function(result) { // (***)
alert(result); // 2
return result * 2;
}).then(function(result) {
alert(result); // 4
return result * 2;
});
the result
will sequentially pass to the next promise object. And chain will be strictly executed one by one. The same we have in current PR, we have a variable with promise and on the next iteration of the cycle we're creating a new promise and call then on it.
Visually it'll look like in the picture:
But if we'll have smth like this:
let promise = new Promise(function(resolve, reject) {
setTimeout(() => resolve(1), 1000);
});
promise.then(function(result) {
alert(result); // 1
return result * 2;
});
promise.then(function(result) {
alert(result); // 1
return result * 2;
});
promise.then(function(result) {
alert(result); // 1
return result * 2;
});
We'll have a situation, when the first promise is resolved ignoring other passed promises.
What about replacing collection with promises with the chain, there is a simple reason, we can call synchronize twice on the same folder and we can avoid simultaneous call on the same folder, we need to process two deltas one by one.
* Prevent NPE after applying the refactoring session Signed-off-by: Vladyslav Zhukovskyi <[email protected]> * Formatting issue Signed-off-by: Vladyslav Zhukovskyi <[email protected]> * Avoid StaleElementException in selenium tests Signed-off-by: Vladyslav Zhukovskyi <[email protected]>
What does this PR do?
Fix an issue with apply refactoring session. After applying the session server responses with bunch of changes (type, old path, new path), but in changes there a may be en nullable change, so this PR adds additional check for
null
in refactoring updater component.Signed-off-by: Vladyslav Zhukovskyi [email protected]
What issues does this PR fix or reference?
#4979
Changelog
Prevent NPE after applying the refactoring session
Release Notes
N/A
Docs PR
N/A