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

Fix file watching for unix #150

Merged
merged 2 commits into from May 14, 2018

Conversation

Projects
None yet
4 participants
@eatkins
Copy link
Contributor

commented May 10, 2018

I inadvertently forgot to reset the watch key after reading the events.
This would cause the EventMonitor to miss watch events because the watch
service stopped posting events to the full key. I looked at the jdk
source code and it looks like each watch key can hold up to 512 events,
so I added a test to ensure that after adding 1000 files, watch events
were still triggered. The test failed before the change to EventMonitor.
I also realized that I reintroduced the bug fixed by @oneill in
881e835 so I fixed that as well.

@eatkins eatkins force-pushed the eatkins:linux-watch-fix branch 4 times, most recently from 2f89362 to 12344a3 May 10, 2018

@eed3si9n
Copy link
Member

left a comment

LGTM
Thanks for the tests too

@eatkins eatkins force-pushed the eatkins:linux-watch-fix branch 6 times, most recently from df6cbe9 to 66994c2 May 10, 2018

Ethan Atkins
Fix file watching for linux
I inadvertently forgot to reset the watch key after reading the events.
This would cause the EventMonitor to miss watch events because the watch
service stopped posting events to the full key. I looked at the jdk
source code and it looks like each watch key can hold up to 512 events,
so I added a test to ensure that after adding 1000 files, watch events
were still triggered. The test failed before the change to EventMonitor.
I also realized that I reintroduced the bug fixed by @oneill in
881e835 so I fixed that as well.

@eatkins eatkins force-pushed the eatkins:linux-watch-fix branch from 66994c2 to 228a880 May 10, 2018

Ethan Atkins
Don't clear events in reset()
We already drain the events in pollEvents. Clearing in reset() can cause
events to be missed (which manifested as flaky tests).

@eatkins eatkins force-pushed the eatkins:linux-watch-fix branch from 228a880 to 70b4c74 May 10, 2018

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2018

Yeah, I was pretty disappointed that I didn't manually test this change fully under linux so I wanted to make sure that there was a regression test. The most annoying part is that there is very similar code in close watch that did the right thing but I didn't refer to it when I implemented the EventMonitor.

@dwijnand dwijnand added this to the 1.1.7 milestone May 14, 2018

@dwijnand dwijnand merged commit 49e8c80 into sbt:1.1.x May 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dwijnand dwijnand modified the milestones: 1.1.7, 1.1.9 May 14, 2018

@OlegYch

This comment has been minimized.

Copy link
Contributor

commented May 14, 2018

i tried this on windows and it doesn't seem to make any difference (compared to sbt 1.1.5) - ~ works only once, and then all modifications are ignored until i relaunch it

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

@OlegYch How did you test this change? Did you build an entirely new version of sbt with the updated io dependency? I ask because I've tried making my project depend on a new version of io via library dependencies and that is not sufficient to get sbt to use the new io. At any rate, I'm downloading a windows iso to see if I can reproduce the bug on a windows vm.

@eatkins eatkins deleted the eatkins:linux-watch-fix branch May 14, 2018

@OlegYch

This comment has been minimized.

Copy link
Contributor

commented May 14, 2018

the added tests seems to fail too https://travis-ci.org/sbt/io/jobs/378830446 , though only on travis

@OlegYch

This comment has been minimized.

Copy link
Contributor

commented May 14, 2018

@eatkins no, i built io locally and then added
libraryDependencies += "org.scala-sbt" %% "io" % "1.1.9-c3c8848d479bee7f0c9004c505e08631f2575a8a-SNAPSHOT" to one of my projects project/plugins.sbt
confirmed it was in effect by doing ;reload plugins; show fullClasspath

@OlegYch

This comment has been minimized.

Copy link
Contributor

commented May 14, 2018

btw any idea why this https://github.com/sbt/io/blob/1.1.x/build.sbt#L15 says 1.1.4 and not 1.1.8 which is the latest published version?

@OlegYch

This comment has been minimized.

Copy link
Contributor

commented May 14, 2018

@eatkins you're right simple libdeps override doesn't work, even if fullClasspath says otherwise
i rebuilt sbt and your solution works as intended on windows too, sorry for the noise
test failure on travis is a concern though

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

Agreed about travis. I am struggling to reproduce, but it's possible that it might be fixed by: 5fc1e46

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

Also, on the plus side, I now have a windows vm for testing which may be useful.

@eatkins eatkins referenced this pull request May 19, 2018

Closed

Triggered Execution / Watching Broken in 1.1.5 on Linux #4167

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.