Skip to content
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

Merged
merged 3 commits into from Nov 22, 2017
Merged

Conversation

vzhukovs
Copy link
Contributor

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 vzhukovs@redhat.com

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

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@vzhukovs vzhukovs added kind/bug Outline of a bug - must adhere to the bug report template. severity/P2 Has a minor but important impact to the usage or development of the system. team/plugin labels Nov 20, 2017
@vzhukovs vzhukovs added this to the 5.21.0 milestone Nov 20, 2017
@vzhukovs vzhukovs self-assigned this Nov 20, 2017
@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 20, 2017
@vzhukovs
Copy link
Contributor Author

ci-build

@vzhukovs
Copy link
Contributor Author

ci-test

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@codenvy-ci
Copy link

Build # 4332 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4332/ to view the results.

@vzhukovs
Copy link
Contributor Author

ci-build

@codenvy-ci
Copy link

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@codenvy-ci
Copy link

@vzhukovs
Copy link
Contributor Author

ci-test

@codenvy-ci
Copy link

@@ -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);
Copy link
Contributor

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?

Copy link
Contributor Author

@vzhukovs vzhukovs Nov 21, 2017

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.

@vzhukovs vzhukovs merged commit 31bdd48 into master Nov 22, 2017
@vzhukovs vzhukovs deleted the che#4979 branch November 22, 2017 12:28
@riuvshin riuvshin modified the milestones: 5.21.0, 5.22.0 Nov 22, 2017
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 22, 2017
JPinkney pushed a commit to JPinkney/che that referenced this pull request Nov 29, 2017
* Prevent NPE after applying the refactoring session

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>

* Formatting issue

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>

* Avoid StaleElementException in selenium tests

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template. severity/P2 Has a minor but important impact to the usage or development of the system.
Projects
No open projects
IDE
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

7 participants