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

Do no watch parent directory of `dirs` #33822

Merged
merged 1 commit into from Dec 21, 2018

Conversation

Projects
None yet
6 participants
@y-yagi
Copy link
Member

y-yagi commented Sep 8, 2018

EventedFileUpdateChecker will search the parent directory if the specified directory does not exist.

Since test/mailers/previews is included in the watch target by default, if there is no test directory (e.g. using rspec), the Rails root directory will be included in the watch target.

$ rails new app
$ cd app
$ ./bin/rails r "p Rails.application.reloaders.last.send(:directories_to_watch).include?(Rails.root)"
false
$ rm -rf test
$ ./bin/rails r "p Rails.application.reloaders.last.send(:directories_to_watch).include?(Rails.root)"
true

This causes node_modules to be included in watch target. Adding parent directories to watch target may include unexpected directories. I think it should be avoided.

Related to #32700.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Sep 12, 2018

I think we'd need the EventedFileUpdateChecker to poll for the existence of any paths it's omitting here: the reason it currently watches the parents [and ultimately, the root] is that someone could create that directory, and then put a file in it, and if we don't notice.. that's bad.

@y-yagi

This comment has been minimized.

Copy link
Member Author

y-yagi commented Sep 14, 2018

I have some doubt whether the case that the directory will be added later is actually there, but indeed I think that the point is right. Thanks.

However, I want to prevent node_modules from being watched.
For example, in the case of Guard, this problem can be avoided by explicitly specifying the directory to be watched, but in the current EventedFileUpdateChecker this can not be done.
https://github.com/guard/listen/wiki/Duplicate-directory-errors#case-of-a-guardfile

Therefore,

  • Add config to specify directory to ignore
  • Ignore node_modules directory by default in EventedFileUpdateChecker(It seems that listen have the plan. guard/listen#338)

I'd like to do one of the fixes, what do you think?

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 14, 2018

Let's go with Add config to specify directory to ignore and have node_modules there by default.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Dec 14, 2018

Sorry, to be clear, I wasn't claiming the current behaviour is necessary -- I was suggesting a third option:

  • Use events to watch the directories that exist, and use polling to check that any initially-absent ones haven't appeared.
@dhh

This comment has been minimized.

Copy link
Member

dhh commented Dec 14, 2018

Whatever we go with, it would be great to have something at least temporary on rails/master. I can't record the video for Action Mailbox while this issue is there present 😄. (Well, at least not using rails/master).

@y-yagi

This comment has been minimized.

Copy link
Member Author

y-yagi commented Dec 15, 2018

@matthewd Does "polling" mean listen's polling adapter, right?

In my understanding, since the polling adapter cannot be specified for directories that do not exist, I think that it is necessary to watch the parent directory (probably the root of the Rails application).
In that case, all of the files of node_modules are subject to polling, and the load does not matter?

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Dec 15, 2018

@y-yagi I was thinking something along these lines: https://gist.github.com/matthewd/820e2100f45aa1011746ebec025a3acc

(Totally untested, just a sketch.)

@y-yagi y-yagi force-pushed the y-yagi:do_not_check_parents_dor_directories branch from bf5c0a9 to f36dcee Dec 17, 2018

@y-yagi

This comment has been minimized.

Copy link
Member Author

y-yagi commented Dec 17, 2018

@matthewd Thanks for clearing. I updated the PR with that approach.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Dec 17, 2018

Awesome, thanks! ❤️

Do you think it's worth adding a test that we don't watch a node_modules-like sibling of a missing directory? Or maybe that's too specific to the old behaviour. 🤔

@boot_mutex.synchronize do
if @pid != Process.pid
if @pid != Process.pid
@boot_mutex.synchronize do

This comment has been minimized.

@matthewd

matthewd Dec 17, 2018

Member

I sneaky-edited which gist my comment was pointing to... just after posting I realised this needs to re-check the conditional inside the lock: Process.pid can't have changed, but @pid might've been fixed by another thread that did the following while we were waiting for the mutex.

Do not add parent directory to file system monitoring
`EventedFileUpdateChecker` will search the parent directory if the
specified directory does not exist.

Since `test/mailers/previews` is included in the watch target by default,
if there is no test directory (e.g. using `rspec`), the Rails root directory
will be included in the watch target.

```
$ rails new app
$ cd app
$ ./bin/rails r "p Rails.application.reloaders.last.send(:directories_to_watch).include?(Rails.root)"
false
$ rm -rf test
$ ./bin/rails r "p Rails.application.reloaders.last.send(:directories_to_watch).include?(Rails.root)"
true
```

This causes `node_modules` to be included in watch target. Adding parent
directories to watch target may include unexpected directories.

In order to avoid this, fixed that parents of nonexistent directories are
not added to the watch targets, instead checking that the directory
exists when checking changes.

Related to #32700.

[Matthew Draper & Yuji Yaginuma]

@y-yagi y-yagi force-pushed the y-yagi:do_not_check_parents_dor_directories branch from f36dcee to caa3cc8 Dec 17, 2018

@y-yagi

This comment has been minimized.

Copy link
Member Author

y-yagi commented Dec 17, 2018

Do you think it's worth adding a test that we don't watch a node_modules-like sibling of a missing directory?

Thank you for pointing out. I think that test should be. I updated.

@y-yagi y-yagi merged commit 1d21ad9 into rails:master Dec 21, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@y-yagi y-yagi deleted the y-yagi:do_not_check_parents_dor_directories branch Dec 21, 2018

@matthewd matthewd referenced this pull request Feb 13, 2019

Merged

Zeitwerk integration #35235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment