[Filesystem] Add watch method to watch filesystem for changes #31462

pierredup · 2019-05-10T09:54:42Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	TBD

Add a watch method to the Filesystem component to monitor the filesystem for changes, and executing a callback function whenever a change is detected.

The watch method can take any parameter and uses a resource locator to transform a path to a resource. The resource can then be monitored for any changes. When a directory is monitored, you can get changed events when a file in the directory changes, when a new file is created or when an existing file is deleted. It also tries to make usage of Inotify is the extension is installed.

Example usage:

$fs = new Filesystem();

$fs->watch('/path/to/some/directory', function ($file, $event) {
    if ($event === FileChangeEvent::FILE_CREATED) {
        echo $file.' was created!';
    }
});

If you want to bail out of the watch events, the callback can return false to stop the process.

$fs = new Filesystem();

$fs->watch('/path/to/some/directory', function ($file, $event) {
    if ($event === FileChangeEvent::FILE_DELETED) {
        echo $file.' was deleted!';
        return false; // <- Will stop the watch process and continue with execution of the the rest of the application
    }
});

ro0NL · 2019-05-10T16:03:40Z

ref #4605, perhaps some relevant info is there still.

src/Symfony/Component/Filesystem/Watcher/Resource/DirectoryResource.php

src/Symfony/Component/Filesystem/Watcher/FileChangeWatcher.php

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php

nicolas-grekas · 2019-09-27T16:10:37Z

@pierredup up to finish this one?

pierredup · 2019-09-30T08:37:53Z

Appveyor failure is unrelated, this is ready for review

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php

michaljusiega · 2019-10-10T20:34:11Z

It is possible to merge this PR in near feature ?

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php

nicolas-grekas · 2019-11-05T17:38:49Z

src/Symfony/Component/Filesystem/Filesystem.php

+     *
+     * @param mixed    $path     The path to watch for changes. Can be a path to a file or directory, iterator or array with paths
+     * @param callable $callback The callback to execute when a change is detected
+     * @param float    $timeout  The time in milliseconds to wait between checking for changes (defaults to 1000 when inotify is not available)


Sorry I wasn't clear enough in my suggestion here: I meant an idle timeout - ie if nothing happens for that duration, we should quit the while loop in implementations.
But as long as something happens during the interval, we should continue looping.
By default or null should mean : forever (as is does now already - configuring the pause timeout doesn't provide any useful behavior from the outside)

Can you clarify how is such timeout useful? As a user that looks highly undesired for watcher to terminate even after one hour of no changes. I wish watcher to run until I terminate it. More useful would be original interpretation of timeout.

lyrixx · 2019-11-26T13:04:52Z

I repeat the question. Is it possible to merge this PR in 5.1 i.e ?

If the feature is accepted and finished on time, it will be merged in 5.1, yes

For what is worth, I'm 👍 for the feature. But I did not read the code in deep. I just saw you are using inotify, and this is the way to go

BTW, did you see https://github.com/flint/Lurker ? may be there is some bits of code to grab

pierredup · 2019-11-26T13:07:29Z

I have just pushed the last changes, so this is ready for another round of review.

BTW, did you see https://github.com/flint/Lurker ? may be there is some bits of code to grab

I did borrow some ideas from there yes.

src/Symfony/Component/Filesystem/Watcher/FileChangeEvent.php

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php

src/Symfony/Component/Filesystem/Watcher/Resource/FileResource.php

pierredup · 2020-03-26T07:45:38Z

Final comments have been addressed and I rebased from master. This is ready again for review. Travis failure is unrelated.

pierredup · 2020-04-29T09:37:03Z

Ping @nicolas-grekas, I would appreciate another review here and hope to get this merged into 5.1

ostrolucky

Thank you. You always come up with most interesting features to contribute. I had this on my to-do list for a while and finally got around to properly review it (didn't test it though). Unfortunately, this doesn't look ready.

src/Symfony/Component/Filesystem/Tests/Watcher/FileSystemWatchTest.php

src/Symfony/Component/Filesystem/Watcher/WatcherInterface.php

src/Symfony/Component/Filesystem/Tests/Watcher/FileSystemWatchTest.php

ostrolucky · 2020-05-05T11:02:03Z

src/Symfony/Component/Filesystem/Tests/Watcher/Resource/ArrayResourceTest.php

+        touch($file, time() + 1);
+
+        $this->assertEquals([new FileChangeEvent($file, FileChangeEvent::FILE_CHANGED)], $resource->detectChanges());
+        $this->assertSame([], $resource->detectChanges());


You are testing here behaviour of FileResource, not ArrayResource, which is confusing. Changing return values on subsequent executions of detectChanges is FileResource specific defined behaviour. If these are unit tests, they should test only behaviour defined by ArrayResource itself, or be renamed to something else, since they aren't unit tests.

Same applies to all of the test classes in this namespace below.

You are testing here behaviour of FileResource, not ArrayResource

I don't understand this part. This is checking that ArrayResource correctly returns the detected changes for all the resources defined. The only other option is to create mocks for the changed events here, but I prefer relying on concrete implementations rather than mocking

I explained it at

Changing return values on subsequent executions of detectChanges is FileResource specific defined behaviour.

This is not something ArrayResource does, so this test case shouldn't explicitly test it. If you want this to be unit test, then indeed you need to provide mock instead of FileResource and assert that ArrayResource calls it when expected, because that's all it does. It won't return different results on subsequent calls for different implementations of decorated resource instance.

Or we can have generic test case like I explain in some other comment, then this is no longer important because there will be no ArrayResource specific test code of these, it will just test these things for consistency sake.

ostrolucky · 2020-05-05T11:27:00Z

src/Symfony/Component/Filesystem/Tests/Watcher/Resource/FileResourceTest.php

+
+        touch($file, time() + 1);
+
+        $this->assertEquals([new FileChangeEvent($file, FileChangeEvent::FILE_CHANGED)], $resource->detectChanges());


Copying same code over and over is a sign it needs extraction. Reducing PR size is also needed to incentive people to review it, larger PRs put people off reviewing it. I would suggest to have abstract test class where most of this code is defined once only and subclasses should define only target class and file location - in other words, only code that differs. Another side benefit is guarantee that all subclasses are tested for all operations. Currently it seems only DirectoryResource is fully tested.

Reducing PR size is also needed to incentive people to review it, larger PRs put people off reviewing it

For some people maybe, but how do other big features than make its way into the codebase?
Abstracting a single line to be re-used does not seem to have a big benefit.

Currently it seems only DirectoryResource is fully tested.

How so? If you think there are test cases missing, let me know and I will gladly add more tests

Abstracting a single line to be re-used does not seem to have a big benefit.

It's not a single line. It's everything except two lines of code like I mentioned at

subclasses should define only target class and file location

Pretty much everything else is currently copy pasted

How so? If you think there are test cases missing, let me know and I will gladly add more tests

testCreateFile, testDeleteFile are missing from FileResourceTest, ArrayResourceTest. I don't want more code to be copy pasted though.

ostrolucky · 2020-05-05T13:35:14Z

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php

+        if ($isDir) {
+            $watchers[] = inotify_add_watch($inotifyInit, $path, IN_CREATE | IN_DELETE | IN_MODIFY);
+
+            foreach ($this->scanPath("$path/*") as $path) {


I wonder how does this work in https://github.com/flint/Lurker, because upon quick inspection, not only I don't see this mechanism there, but also no inotify_add_watch. I'm wondering this because recursively iterating over all folders and creating inotify watcher for each doesn't look efficient. I expect startup times in any moderately sized projects to be long doing it like this.

not only I don't see this mechanism there, but also no inotify_add_watch

inotify_add_watch is used here

Checking recursively can maybe be turned off by default and enabled by either a flag or a new method watchRecursive

inotify_add_watch is used here

Thanks, GH didn't find it for me.

Checking recursively can maybe be turned off by default and enabled by either a flag or a new method watchRecursive

I don't mean to have ability to turn recursivity off, I am just trying to figure if there isn't smarter way to go around handling recursivity. First thing we need to clear up is: Is it absolute requirement for inotify PHP extension to create watcher for every subdirectory, instead of just parent directory?

src/Symfony/Component/Filesystem/Watcher/Resource/DirectoryResource.php

ostrolucky · 2020-05-05T13:55:22Z

src/Symfony/Component/Filesystem/Watcher/Resource/DirectoryResource.php

+        // Check if any files has been added
+        foreach (array_keys($currentFiles) as $path) {
+            if (!isset($this->files[$path])) {
+                $this->files = $currentFiles;


I don't understand how is DirectoryResource designed to work. If there were multiple changes, e.g. file created and another file deleted, file creation will be detected by first foreach, then $this->files will be overwritten and following loops will no longer detect other changes.

ostrolucky · 2020-05-05T14:03:26Z

src/Symfony/Component/Filesystem/Watcher/Resource/Locator/FileResourceLocator.php

+
+            $paths = glob($path, \defined('GLOB_BRACE') ? GLOB_BRACE : 0);
+
+            if (1 === \count($paths)) {


Why do we care here if array has single path and not let ArrayResource do it's job? ArrayResource surely works with single value array being passed in too?

Small optimization. If we know there is only one file, then we don't need the loop in ArrayResource and can just check the file directly

Right, but I would say super premature optimization. This isn't really worth it for increase of cyclomatic complexity.

YaFou

Promising pull request! Can't wait to release this new feature 🚀 .

src/Symfony/Component/Filesystem/Filesystem.php

YaFou · 2020-06-23T07:20:46Z

src/Symfony/Component/Filesystem/Watcher/Resource/DirectoryResource.php

+                $events = array_merge($events, $event);
+            }
+        }
+


I think you have to move all $this->files = $currentFiles here to avoid overwriting initial state and miss changes.

YaFou · 2020-06-23T07:25:24Z

src/Symfony/Component/Filesystem/Filesystem.php

+     *
+     * @throws \InvalidArgumentException|IOException
+     */
+    public function watch($path, callable $callback, float $timeout = null)


It would be nice to define events we want to listen (by default creating, changing, and deleting).

The events are passed to the callback function, so you can skip certain events already by adding an if statement in the callback

Some instructions take time to be executed and limit changes we want to watch can help to reduce useless time. Also, the change must in the watcher code to do it.

YaFou · 2020-06-23T07:33:57Z

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php

+ *
+ * @internal
+ */
+final class INotifyWatcher implements WatcherInterface


No tests for this watcher?

fabpot · 2020-09-06T07:28:07Z

It took me some time to have a look at this PR. Sorry for that but I wasn't sure how I felt about it. On one hand, that would be great to have something like this in Symfony, but in the other hand, it's like opening a Pandora box. I've worked with similar systems in other languages, and it's really hard to get ti right and cross platform. The fallback in PHP is going to be very slow/CPU intensive and I would not recommend it except in very specific conditions. inotify is only for Linux, so Windows and MacOS will always use the fallback, which is a big problem for me. Adding support for Windows and MacOS is not trivial either as
the equivalent of inotify work very differently (especially when it comes to watching directories). Also, I think it would deserve a dedicated component.

With all that said, and after thinking about it a lot, I think closing it is the best option for now as it's not ready for prime time yet.

carsonbot added Status: Needs Review Feature labels May 10, 2019

pierredup mentioned this pull request May 10, 2019

Add watch option to lint commands #26120

Closed

pierredup changed the title ~~Add watch method to watch filesystem for changes~~ [Filesystem] Add watch method to watch filesystem for changes May 10, 2019

pierredup force-pushed the filesystem-watch branch from 0fdfa5a to 7ef4b10 Compare May 10, 2019 09:58

staabm reviewed May 11, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Watcher/Resource/DirectoryResource.php Outdated Show resolved Hide resolved

nicolas-grekas modified the milestones: 4.3, next May 11, 2019

miniyarov reviewed May 12, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Watcher/FileChangeWatcher.php Show resolved Hide resolved

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php Show resolved Hide resolved

nicolas-grekas mentioned this pull request Sep 27, 2019

[FrameworkBundle] Add --watch option to cache:clear command #32764

Closed

pierredup force-pushed the filesystem-watch branch from 7ef4b10 to 4d7dae5 Compare September 30, 2019 07:47

pierredup changed the base branch from master to 4.4 September 30, 2019 07:51

pierredup force-pushed the filesystem-watch branch 3 times, most recently from cce66d2 to 1cbfd18 Compare September 30, 2019 08:22

nicolas-grekas reviewed Sep 30, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php Outdated Show resolved Hide resolved

pierredup force-pushed the filesystem-watch branch from 1cbfd18 to 2a556d3 Compare September 30, 2019 10:22

nicolas-grekas reviewed Sep 30, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php Outdated Show resolved Hide resolved

pierredup force-pushed the filesystem-watch branch from 418d1d6 to 5d6ada4 Compare October 1, 2019 09:25

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

maidmaid reviewed Nov 1, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php Outdated Show resolved Hide resolved

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Nov 5, 2019

View reviewed changes

nicolas-grekas modified the milestones: 4.4, next Nov 8, 2019

pierredup requested review from dunglas, lyrixx, sroze and xabbuh as code owners November 26, 2019 12:13

pierredup changed the base branch from 4.4 to master November 26, 2019 12:14

pierredup force-pushed the filesystem-watch branch from 4131719 to 73d84a2 Compare November 26, 2019 12:22

OskarStark reviewed Nov 27, 2019

View reviewed changes

src/Symfony/Component/Filesystem/Watcher/FileChangeEvent.php Outdated Show resolved Hide resolved

src/Symfony/Component/Filesystem/Watcher/INotifyWatcher.php Outdated Show resolved Hide resolved

src/Symfony/Component/Filesystem/Watcher/Resource/FileResource.php Show resolved Hide resolved

pierredup force-pushed the filesystem-watch branch from bdcd781 to de386b2 Compare March 26, 2020 06:59

pierredup force-pushed the filesystem-watch branch 2 times, most recently from 868c3f5 to fc8c244 Compare April 21, 2020 08:51

pierredup requested a review from nicolas-grekas April 22, 2020 08:34

pierredup force-pushed the filesystem-watch branch from fc8c244 to c7036c9 Compare April 29, 2020 08:14

pierredup added 4 commits May 5, 2020 12:14

Add watch method to watch filesystem for changes

3c9f3e7

Add recursive directory handling to inotify watcher

78c9969

Make internal classes final and additional return types

86ae251

Fix CS

23f09e6

pierredup force-pushed the filesystem-watch branch from c7036c9 to 23f09e6 Compare May 5, 2020 10:15

ostrolucky suggested changes May 5, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 5, 2020

Update some tests

5b31568

YaFou suggested changes Jun 23, 2020

View reviewed changes

fabpot closed this Sep 6, 2020

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020


		touch($file, time() + 1);

		$this->assertEquals([new FileChangeEvent($file, FileChangeEvent::FILE_CHANGED)], $resource->detectChanges());


		$paths = glob($path, \defined('GLOB_BRACE') ? GLOB_BRACE : 0);

		if (1 === \count($paths)) {

Uh oh!

[Filesystem] Add watch method to watch filesystem for changes #31462

[Filesystem] Add watch method to watch filesystem for changes #31462

Uh oh!

Conversation

pierredup commented May 10, 2019

Uh oh!

ro0NL commented May 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolas-grekas commented Sep 27, 2019

Uh oh!

pierredup commented Sep 30, 2019

Uh oh!

Uh oh!

Uh oh!

michaljusiega commented Oct 10, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyrixx commented Nov 26, 2019

Uh oh!

pierredup commented Nov 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pierredup commented Mar 26, 2020

Uh oh!

pierredup commented Apr 29, 2020

Uh oh!

ostrolucky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ostrolucky May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YaFou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ostrolucky May 5, 2020 •

edited

Loading