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

Remove spring-watcher-listen from default Gemfile #36377

Merged
merged 1 commit into from Dec 17, 2019

Conversation

y-yagi
Copy link
Member

@y-yagi y-yagi commented Jun 2, 2019

spring-watcher-listen watch application root by default.
https://github.com/jonleighton/spring-watcher-listen/blob/c4bfe15805867e229588e4b776a7b87438137094/lib/spring/watcher/listen.rb#L58

This is necessary to watch the file (e.g. .ruby-version) in the application root.

By this node_modules also be watched, and it is a possibility to be
shown a warning by listen.
Related to #32700, #34912, rails/webpacker#1990.

listen watches directory recursive by default, and it cannot avoid it.
guard/listen#111
So If this warning happens, the only workaround the user can do is remove the gem.

The issue is likely to occur more frequently in Rails 6 because rails new runs webpacker:install by default. Because of such a state, I think that we should not recommend to use spring-watcher-listen.

Spring has polling watcher, restart process works without this
spring-watcher-listen. Because of polling base, CPU load may be higher than listen base. Still I think that it is better than the warning comes out.

`spring-watcher-listen` watch application root by default.
https://github.com/jonleighton/spring-watcher-listen/blob/c4bfe15805867e229588e4b776a7b87438137094/lib/spring/watcher/listen.rb#L58

This is necessary to watch the file (e.g. `.ruby-version`) in the
application root.

By this `node_modules` also be watched, and it is a possibility to be
shown a warning by `listen`.
Related to rails#32700, rails#34912, rails/webpacker#1990.

`listen` watches directory recursive by default, and it cannot avoid it.
guard/listen#111

So If this warning happens, the only workaround the user can do is remove
the gem.

The issue is likely to occur more frequently in Rails 6 because
`rails new` runs `webpacker:install` by default. Because of such a
state, I think that we should not recommend to use
`spring-watcher-listen`.

Spring has polling watcher, restart process works without this
`spring-watcher-listen`.
Because of polling base, CPU load may be higher than listen base. Still
I think that it is better than the warning comes out.
@rails-bot rails-bot bot added the railties label Jun 2, 2019
Copy link
Member

@schneems schneems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, who added it? Can we bring them in on the discussion?

@y-yagi
Copy link
Member Author

y-yagi commented Jun 2, 2019

Can we bring them in on the discussion?

Good point. Thanks.

@fxn You added spring-watcher-listen by 00a5eb6. How do you think about this PR?

@fxn
Copy link
Member

fxn commented Jun 2, 2019

Hey, quick heads up.

Reason was performance in large projects, where walking the project tree constantly may not be cheap. Think Shopify.

Same reason Rails introduced reloading based on listen, to be reactive and avoid polling.

@fxn
Copy link
Member

fxn commented Jun 2, 2019

In principle, as a first thought, I think I'd open an issue in the gem to discuss it. listen is able to ignore patterns, so we could see if the gem can be more smart about what to watch in Rails 6 apps.

WDTY?

@y-yagi
Copy link
Member Author

y-yagi commented Jun 2, 2019

Unfortunately, Listen's ignore option does not really ignore directories. The directories are still watched by the native adapter, and events are only filtered after they are fired. Therefore, properly specifying the ignore option does not solve this issue.
Ref: https://github.com/guard/listen/wiki/Duplicate-directory-errors#how-to-correctly-prevent-the-error-in-your-project, guard/listen#274, guard/listen#338

@y-yagi
Copy link
Member Author

y-yagi commented Jun 3, 2019

Reason was performance in large projects, where walking the project tree constantly may not be cheap.

Thanks for your description. In my understanding, the role of spring watcher is to watch only files that want to monitor for a restart of spring. As this is only a part of the files such as initializers and database.yml, I think that it will not be an issue unless it is really large.

@jordan-brough
Copy link
Contributor

jordan-brough commented Jun 3, 2019

@y-yagi removing spring-watcher-listen only partially resolves the problem for me.

Removing spring-watcher-listen does eliminate the warnings when starting a rails console (spring stop && rails console) in my example app.

However, see this branch of my example app where spring-watcher-listen is removed but I still see warnings when I invoke reload! within a Rails console:

Show repro

$ spring stop && rails console
Spring stopped.
Running via Spring preloader in process 30245
Loading development environment (Rails 6.0.0.rc1)
001 > reload!
Reloading...
=> true
002 >         ** ERROR: directory is already being watched! **

        Directory: /Users/jordan/repos/example-already-watched/node_modules/.bin/compression-webpack-plugin

        is already being watched through: /Users/jordan/repos/example-already-watched/node_modules/compression-webpack-plugin

        MORE INFO: https://github.com/guard/listen/wiki/Duplicate-directory-errors
        ** ERROR: directory is already being watched! **

        Directory: /Users/jordan/repos/example-already-watched/node_modules/@rails/webpacker/node_modules/.bin/compression-webpack-plugin

        is already being watched through: /Users/jordan/repos/example-already-watched/node_modules/compression-webpack-plugin

        MORE INFO: https://github.com/guard/listen/wiki/Duplicate-directory-errors

@fxn
Copy link
Member

fxn commented Jun 3, 2019

Hey, need to work on a presentation for tomorrow, I'll eventually come back to this issue.

@y-yagi
Copy link
Member Author

y-yagi commented Jun 3, 2019

@jordan-brough Thanks for your report.
That is another issue. That issue is that CacheExpiry uses a watcher without specifying a target when reload!. I will investigate.

@watcher = @watcher_class.new([], watched_dirs) do

y-yagi added a commit to y-yagi/rails that referenced this pull request Jun 3, 2019
Currently, `clear_cache_if_necessary` is executed even if view paths are
not set like `rails console`.
If the watcher class is `EventedFileUpdateChecker` and the watch
directories are empty, the application root directory will watch. This
is because listen uses the current directory as the default watch directory.
https://github.com/guard/listen/blob/8d85b4cd5788592799adea61af14a29bf2895d87/lib/listen/adapter/config.rb#L13

As a result, `node_modules` also watch. This cause a warning of `listen`.
Ref: rails#36377 (comment)
@matthewd
Copy link
Member

matthewd commented Jun 4, 2019

I don't know much about spring-watcher-listen, but could we teach it that it's dangerous to listen at the root, and that it should instead install listeners on interesting subdirectories, and then use polling for the remaining top-level files?

@y-yagi
Copy link
Member Author

y-yagi commented Jun 6, 2019

it should instead install listeners on interesting subdirectories, and then use polling for the remaining top-level files?

I tried to implement this, maybe can do. y-yagi/spring-watcher-listen@c3c697d
However, since the top-level file is monitored by default, the polling thread is executed by default, so I think that CPU load may still increase.
(Of course if don't watch top-level files, I think there are benefits)

@rafaelfranca
Copy link
Member

If this by default will increase CPU why should not we remove it? I think the benefits that it may bring doesn't overweight the problems that it will cause.

@rails-bot
Copy link

rails-bot bot commented Dec 17, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 17, 2019
@rafaelfranca rafaelfranca merged commit 399b7be into rails:master Dec 17, 2019
@eugeneius eugeneius removed the stale label Nov 15, 2020
@ngan
Copy link
Contributor

ngan commented Nov 19, 2020

@y-yagi is there a reason to keep the listen gem in the default Gemfile if we’re taking out spring-watcher-listen?

@rafaelfranca
Copy link
Member

Yes. It is for the evented file updater.

christiannelson added a commit to carbonfive/raygun-rails that referenced this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants