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

Watch improvements #142

Merged
merged 19 commits into from Apr 25, 2018

Conversation

Projects
None yet
5 participants
@eatkins
Copy link
Contributor

commented Apr 12, 2018

This pull request makes some improvements to the source modification watch.

  • The PollingWatchService had a few minor issues that caused transient test failures
  • The MacOSXWatchService did not quite implement the WatchService api correctly (DefaultWatchServiceSpec failed with the watch service set to MacOSXWatchService
  • I re-implement the SourceModificationWatch.watch method to remove the latency between receiving file or user input events and triggering a build (or exiting).

@eed3si9n eed3si9n added the ready label Apr 12, 2018

@eatkins eatkins force-pushed the eatkins:watch-improvements branch 2 times, most recently from c015d80 to 764c5ca Apr 12, 2018

@eed3si9n

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

@eatkins Thanks for the update.
I am going to be busy this week and next for conference, so the review might delay.

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

Sounds good. I have some improvements I want to make anyway.

@eatkins eatkins force-pushed the eatkins:watch-improvements branch 2 times, most recently from aa8ef1e to 8c9c7bf Apr 13, 2018

eatkins pushed a commit to eatkins/sbt that referenced this pull request Apr 13, 2018

Ethan Atkins
Use new EventMonitor in executeContinuously
In sbt/io#142, I add a new api for watching for
source file events. This commit updates sbt to use the new EventMonitor
based api. The EventMonitor has an anti-entropy parameter, so that
multiple events on the same file in a short window of time do not
trigger a build. I add a key to tune it.

The implementation of executeContinuously is pretty similar. The main
changes are that shouldTerminate now blocks (EventMonitor spins up a
thread to check the termination condition) and that the
EventMonitor.watch method only returns a Boolean. This is because
the event monitor contains mutable state. It does, however, have a
state() method that returns an immutable snapshot of the state.

eatkins pushed a commit to eatkins/sbt that referenced this pull request Apr 13, 2018

Ethan Atkins
Bump io
My next commit replaces the implementation of
Watched.executeContinuously using apis that are available only in a
pending io pull request (sbt/io#142).

eatkins pushed a commit to eatkins/sbt that referenced this pull request Apr 13, 2018

Ethan Atkins
Use new EventMonitor in executeContinuously
In sbt/io#142, I add a new api for watching for
source file events. This commit updates sbt to use the new EventMonitor
based api. The EventMonitor has an anti-entropy parameter, so that
multiple events on the same file in a short window of time do not
trigger a build. I add a key to tune it.

The implementation of executeContinuously is pretty similar. The main
changes are that shouldTerminate now blocks (EventMonitor spins up a
thread to check the termination condition) and that the
EventMonitor.watch method only returns a Boolean. This is because
the event monitor contains mutable state. It does, however, have a
state() method that returns an immutable snapshot of the state.

@eatkins eatkins force-pushed the eatkins:watch-improvements branch 2 times, most recently from 9e4d66b to 9d0bc17 Apr 13, 2018

@hepin1989

This comment has been minimized.

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2018

Care to elaborate, @hepin1989?

@dwijnand
Copy link
Member

left a comment

despite its size reviewing this was a real pleasure commit-by-commit.. until the last commit bomb :)

I don't have the bandwidth to review it in detail, but I like the sound of it.

I'll give Eugene a chance to weigh in in case he weighs in differently to me.

@@ -156,7 +156,8 @@ final class Source(
if (includeDirs) DirectoryFilter || includeFilter
else includeFilter

p.startsWith(base.toPath) && inc.accept(p.toFile) && !excludeFilter.accept(p.toFile)
(if (!recursive) p.getParent == base.toPath else p.startsWith(base.toPath)) && inc.accept(
p.toFile) && !excludeFilter.accept(p.toFile)

This comment has been minimized.

Copy link
@dwijnand

dwijnand Apr 23, 2018

Member

if could we add a test for this oversight, it would be nice.

This comment has been minimized.

Copy link
@eatkins

eatkins Apr 23, 2018

Author Contributor

Done.

val base: File,
val includeFilter: FileFilter,
val excludeFilter: FileFilter,
val recursive: Boolean

This comment has been minimized.

Copy link
@dwijnand

dwijnand Apr 23, 2018

Member

I agree! #123 xD

@dwijnand dwijnand requested a review from eed3si9n Apr 23, 2018

Ethan Atkins added some commits Apr 9, 2018

Ethan Atkins
Apply recursive parameter in source filter
Right now, even with the recursive toggle, the Source.accept method will
accept files that are not in the base path.

I also added a simple test suite for Source to verify the basic
semantics.
Ethan Atkins
Bump appleFileEvents
I discovered an issue in the apple-file-events library that could cause
certain api calls to induce a segfault. I fixed the bug upstream:
swoval/swoval@78d9c03
Ethan Atkins
Use the MacOSXWatchService in DefaultWatchServiceSpec
It was an oversight that I didn't make this change when adding the
MacOSXWatchService.
Ethan Atkins
Set file times on register
When registering a new path, we should not trigger events for the files
that already exist in the directory. To fix this, we calculate the last
modified times for all of the files in the newly registered path and add
them to the list.

This fixed spurious test failures that I was having in my anti-entropy
spec.
Ethan Atkins
Add unregister to WatchService
When a watched directroy is deleted, it's good practice to cleanup the
resources associated with it. If it's recreated, we can just
re-register. For binary compatibility reasons, I couldn't add unregister
to WatchService, so I added a trait that gets mixed into all of the
implementations. The method is then accessible in abstract WatchServices
by runtime pattern matching.

This change also has the side effect of preventing the default
WatchService from creating redundant registrations.

Bonus: fix some compiler warnings
Ethan Atkins
Use realPath to register in MacOSXWatchService
Temporary directories on osx generally have a different value for path
vs. realPath (the latter usually has a leading /private). The events
from the apple file system api are real paths, with the leading
/private, so in order to check if the event is for a registered path, we
must register using the real path.
Ethan Atkins
Monitor streams
The apple file system api only offers recursive directory listeners,
which makes it pointless to have a listener for each subdirectory.
Whenever a stream is registered with FileEventsApi and there are
previously registered streams that are subdirectories of this stream,
the FileEventsApi removes the now redundant streams and fires a callback
with the name of each directory whose events are now triggered by the
parent stream. When this callback is fired, the streamId associated with
the stream is no longer valid. In this commit, I start tracking the
registered streams and don't bother registering a subdirectory with the
watch service if one of its parents is already being monitored.
Ethan Atkins
Fire events for base paths
Prior to this commit, if a file event was detected for a base directory,
no event was fired because the parent of the base directory wasn't
monitored.

Bonus: check if the service is open in a few methods and throw a
ClosedWatchServiceException if it isn't.
Ethan Atkins
Defer removal of deleted files in PollingWatchService
I noticed that when a directory was removed, the PollingWatchService
didn't always report file events. This was because sometimes the parent
directory was removed from the `watched` map before the delete file was
handled in the loop. This caused the loop to treat it like an
unmonitored file.

@eatkins eatkins force-pushed the eatkins:watch-improvements branch from 9d0bc17 to 6afe71d Apr 23, 2018

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2018

Thanks @dwijnand. Yeah, that last commit was hard to split up into bite sized chunks. Apologies for the review overhead.

@eatkins eatkins force-pushed the eatkins:watch-improvements branch from 692c461 to 5003887 Apr 24, 2018

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

Figured out Travis. Somewhat frustratingly, I'm having trouble getting the PollingWatchServiceSpec to pass on osx (but the DefaultWatchServiceSpec works pretty reliably well). At any rate, I'm sure I'll get the tests working tomorrow.

@eatkins eatkins force-pushed the eatkins:watch-improvements branch from 1c28048 to 6caa282 Apr 24, 2018

// We give it more time to detect changes.
val (pollDelay, maxWaitTime) = (50.milliseconds, 3.seconds)
val pollDelay =
Option(System.getProperty("io.watch.poll.ms")).fold(50.milliseconds)(_.toInt.milliseconds)

This comment has been minimized.

Copy link
@dwijnand

dwijnand Apr 24, 2018

Member

could you prefix the prop key with sbt. please?

This comment has been minimized.

Copy link
@eatkins

eatkins Apr 24, 2018

Author Contributor

I was only using this for travis debugging. It's gone now. I will keep in mind prefixing keys with sbt. if it comes up again though.

@eatkins eatkins force-pushed the eatkins:watch-improvements branch from 926adee to 0bc6101 Apr 24, 2018

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

I figured out the travis issues with PollingWatchServiceSpec (among other things, the travis osx agents use HFS+, which doesn't have millisecond precision). I also added the scaladocs to EventMonitor as requested. Finally, I renamed EventMonitor.watch to EventMonitor.awaitEvents as suggested by @eed3si9n.

@eatkins eatkins force-pushed the eatkins:watch-improvements branch from 0bc6101 to d7d4cf9 Apr 24, 2018

Ethan Atkins added some commits Apr 11, 2018

Ethan Atkins
Reduce timeouts
These timeouts are all excessively long from what I can tell. This
change makes the whole test suite take ~10 seconds instead of ~45
seconds (the *WatchServiceSpec decline from ~45 seconds to ~2.5 seconds
on my machine). I found, however, that on travis, longer timeouts are
needed because the jvm needs to warm up and the timeouts are not short
enough in that scenario.
Ethan Atkins
Use real path in SourceModificationWatchSpec
The SourceModificationWatchSpecs fail on OSX because temp directories
are reported by the watch service with an additional prefix "/private".
This prefix is added when toRealPath is called on the temp directory.
Ethan Atkins
Make Source fields public
It is a pain for plugin writers that these fields are private. I had to
use reflection to extract them in CloseWatch.

@eatkins eatkins force-pushed the eatkins:watch-improvements branch 2 times, most recently from 70c9b76 to 2af4854 Apr 24, 2018

@eed3si9n
Copy link
Member

left a comment

LGTM

Ethan Atkins added some commits Apr 9, 2018

Ethan Atkins
Reduce watch service latency
There is often noticeable lag between when a file event happens and when
the build is triggered. There is also often latency when the user
presses enter to interrupt the watch. Both are due to the way that
SourceModificationWatch polls for results. It can be removed by having
the main thread block on an event queue that is asynchronously filled by
threads that wait for user input and file events. I made a similar
change in CloseWatch.

The new EventMonitor is where most of the logic lives. It is implemented
as a block box that effectively provides three methods:
1) watch() -- block the main thread until a user input or watched file
   event occurs, returning true if the build was triggered
2) state() -- a snapshot of the current watch state. This is primarily
   for legacy compatibility with sbt, which has a number of methods that
   take the state as input. In practice, all that sbt really needs most
   of the time is the count of the number of build triggers
3) close() -- shutdown any threads or service started up by the event
   monitor

I implemented the EventMonitor as a block box so that it would be
straightforward to change the existing implementation or add new
implementations without having to break forward and backwards
compatibility with sbt. In particular, I can envision adding a second
EventMonitor that uses a persistent file system cache for file events
instead of the ad-hoc cache that currently exists in the
EventMonitorImpl.eventThread.

At the moment, the one implementation is EventMonitorImpl. It spins up
two threads for monitoring user input and the file events. Both write
`EventMonitor.Events` to a concurrent queue that the main thread reads
from. User input events supersede file triggers. When the user hits
enter, the queue is cleared and filled with the exit event. Otherwise,
the event thread polls the watch service for events. When it receives a
file event, it adds the event to a cache of recent events. This cache is
used to prevent multiple builds from being triggered by the same file*.
The eventThread also detects when directories are created or deleted and
registers or unregisters the directory with the watch service as needed.

I also stopped registering all of the files. Only directories are
registered now. The registered files just waste memory.

This commit also adds logging of watch events. Unfortunately, I can't
get the logging to actually work at the debug level due to
sbt/sbt#4097, but once that issue is fixed,
logging with the new EventMonitor should work.

I added a simple test that the anti-entropy works and made some small
adjustments to the existing tests to make them work with the new
implementation.

For backwards compatibility with older versions of sbt, I re-implement
SourceModificationWatch.watch to wrap an EventMonitor that we shutdown
after each trigger. This is also the reason that I had to add two close
methods to EventMonitor -- one that shuts down the watch service, and
one that doesn't.

* When neovim, for example, saves a file. It moves the buffer into the
file location which can trigger a delete and a create event on the file.
Without the anti-entropy timeout, there would be two build triggered for
the same file save.
Ethan Atkins
Make PollingThread.start block until startup
Rather than spinning on PollingThread.initDone in the init() method, I
override the start() method of PollingThread to block until the created
thread is done initializing. This should reduce the average runtime of
init() by 50ms in most cases. I am sure that it doesn't matter, but I
also didn't like reading from thread.initDone directly without
synchronization from a different thread.

For binary compatibility had to provide a getter and setter for
_initDone (each of which I deprecated). I suspect that no one is using
these apis outside of the PollingWatchService file since PollingThread
is private which suggests that it might make sense to just add a mima
filter instead.
Ethan Atkins
Set new file modified times in test
I was having a lot of trouble getting the PollingWatchServiceSpec to
pass on travis with osx and it turns out this is because the osx agents
are using HFS+ not APFS. The HFS+ file system doesn't support
millisecond precision on modified times. To fix this, set the file
modified time to be in the past when writing a new file.
Ethan Atkins
Don't directly expose keys
The keysWithEvents was exposing unsynchronized mutable data. Even though
it was only accessed from a different thread inside of synchronized
blocks, it seems better to guarantee this behavior by adding a helper
accessor that ensures synchronization.

For binary compatibility had to provide a getter (that I deprecated) for
_keysWithEvents. I suspect that no one is using these apis outside of
the PollingWatchService file since PollingThread is private which
suggests that it might make sense to just add a mima filter instead.

@eatkins eatkins force-pushed the eatkins:watch-improvements branch from 2af4854 to b2c0cf6 Apr 25, 2018

Ethan Atkins
Retry setModifiedTimeOrFalse on error
Occasionally on Travis, I get errors where IO.setModifiedTimeOrFalse
throws an exception because utimensat returns an EINVAL. Fixing whatever
is going on in IO.setModifiedTimeOrFalse is beyond the scope of my work,
but I want the travis builds to not be flaky for now.
@dwijnand
Copy link
Member

left a comment

LGTM. Thanks @eatkins!

@dwijnand dwijnand added this to the 1.1.7 milestone Apr 25, 2018

@dwijnand dwijnand merged commit 1bbf099 into sbt:1.1.x Apr 25, 2018

1 check passed

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

@dwijnand dwijnand removed the ready label Apr 25, 2018

@cunei

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

@eatkins Can you please provide a link to a failing Travis run in which you see utimensat() returning EINVAL? According to the man page, there are only a few cases in which that may happen, the most likely being a NULL path name.

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

@eatkins eatkins deleted the eatkins:watch-improvements branch Oct 9, 2018

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.