-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Do no watch parent directory of dirs
#33822
Conversation
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. |
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 Therefore,
I'd like to do one of the fixes, what do you think? |
Let's go with |
Sorry, to be clear, I wasn't claiming the current behaviour is necessary -- I was suggesting a third option:
|
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). |
@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). |
@y-yagi I was thinking something along these lines: https://gist.github.com/matthewd/820e2100f45aa1011746ebec025a3acc (Totally untested, just a sketch.) |
bf5c0a9
to
f36dcee
Compare
@matthewd Thanks for clearing. I updated the PR with that approach. |
Awesome, thanks! ❤️ Do you think it's worth adding a test that we don't watch a |
@boot_mutex.synchronize do | ||
if @pid != Process.pid | ||
if @pid != Process.pid | ||
@boot_mutex.synchronize do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
`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 rails#32700. [Matthew Draper & Yuji Yaginuma]
f36dcee
to
caa3cc8
Compare
Thank you for pointing out. I think that test should be. I updated. |
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. usingrspec
), the Rails root directory will be included in the watch target.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.