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

Check for additional file events in watch event poll #12384

Merged
merged 4 commits into from
Jan 24, 2019
Merged

Conversation

vzhukovs
Copy link
Contributor

@vzhukovs vzhukovs commented Jan 10, 2019

What does this PR do?

This changes proposal adds fix for the problem related to sequential handling file watcher events. The problem is in mechanism of receiving events: https://github.com/eclipse/che/blob/master/wsagent/che-core-api-project/src/main/java/org/eclipse/che/api/watcher/server/impl/FileWatcherService.java#L272. We try to get events and continue work with them but we don't check that in this moment we can receive additional events.

Use case to reproduce such problem: use git repository with the following structure:

.
├── dir
│   ├── file1
│   ├── file2
│   └── file3
└── other.file

clone this repo into che and make outside operation to make the following structure:

.
├── dir
└── other.file

file watcher detects changes and we call https://github.com/eclipse/che/blob/master/wsagent/che-core-api-project/src/main/java/org/eclipse/che/api/watcher/server/impl/FileWatcherService.java#L272 we receive two events about delete file1 and dir, but in a moment we receive rest two events that is not handled for file2 and file3.

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

What issues does this PR fix or reference?

#11648

Release Notes

N/A

Docs PR

N/A

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@vzhukovs vzhukovs requested a review from dkulieshov January 10, 2019 16:41
@vzhukovs vzhukovs requested a review from vparfonov as a code owner January 10, 2019 16:41
@vzhukovs
Copy link
Contributor Author

ci-test

@eclipse-che eclipse-che deleted a comment from che-bot Jan 10, 2019
@che-bot
Copy link
Contributor

che-bot commented Jan 10, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12384
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@vzhukovs
Copy link
Contributor Author

ci-build

@vzhukovs vzhukovs self-assigned this Jan 14, 2019
@vzhukovs vzhukovs added kind/bug Outline of a bug - must adhere to the bug report template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. team/ide severity/P1 Has a major impact to usage or development of the system. e2e-test/failure Issues that is related to a test failures reported by our CI platform and our QE. labels Jan 14, 2019
@vzhukovs vzhukovs added this to the 6.17.0 milestone Jan 14, 2019
@vzhukovs vzhukovs changed the title [WIP] Check for additional file events in watch event poll Check for additional file events in watch event poll Jan 14, 2019
@artaleks9
Copy link
Contributor

artaleks9 commented Jan 15, 2019

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/1408/) doesn't show any regression against this Pull Request.
The bug is not reproduced. It was checked on local machine.

@@ -269,33 +270,35 @@ private void run() {
continue;
}

List<WatchEvent<?>> watchEvents = watchKey.pollEvents();
for (List<WatchEvent<?>> watchEvents = watchKey.pollEvents();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe while statement here make code more readable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean something like this?:

        List<WatchEvent<?>> watchEvents = watchKey.pollEvents();
        while(!watchEvents.isEmpty()) {
          if (suspended.get()) {
            ...
          }

          for (WatchEvent<?> event : watchEvents) {
            ...
          }

          watchEvents = watchKey.pollEvents();
        }

@vzhukovs
Copy link
Contributor Author

ci-build

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

ci-build

@vzhukovs vzhukovs merged commit 67dc2e1 into master Jan 24, 2019
@vzhukovs vzhukovs modified the milestones: 6.17.0, 6.18.0 Jan 24, 2019
@vzhukovs vzhukovs deleted the che#11648 branch January 24, 2019 09:26
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-test/failure Issues that is related to a test failures reported by our CI platform and our QE. kind/bug Outline of a bug - must adhere to the bug report template. severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants