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

Add Listen 2 support #194

Closed
wants to merge 10 commits into from
Closed

Add Listen 2 support #194

wants to merge 10 commits into from

Conversation

thibaudgg
Copy link

Needed if user have Guard 2.0 (that depends on Listen 2.0) in there Gemfile.

@senny Wouldn't be better to keep only Listen and remove spring polling watcher completely?

@senny
Copy link
Member

senny commented Oct 8, 2013

I think it's best to wait for the full release of listen 2 before we ship this PR. I don't think we should depend on beta releases. People running Guard 2 should of course be able to run this PR so they won't get stuck until we upgrade the dependency.

/cc @jonleighton

@thibaudgg
Copy link
Author

@senny I already release listen 2.0 some days ago. :)

@@ -3,4 +3,4 @@ source 'https://rubygems.org'
# Specify your gem's dependencies in spring.gemspec
gemspec

gem 'listen', :require => false
gem 'listen', "~> 2.0.0.beta.2", :require => false
Copy link
Member

Choose a reason for hiding this comment

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

If Listen 2 was released is there a need to specify the beta here?

Copy link
Author

Choose a reason for hiding this comment

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

Good point, fixed!

@thibaudgg
Copy link
Author

Good point, fixed. Please give it a try before merging it, It could have a thread issue somewhere.

@senny
Copy link
Member

senny commented Oct 8, 2013

Did you see that Travis is failing?

@thibaudgg
Copy link
Author

Yep, and it's certainly related to the thread issue. I'll have a look at it this week, I need to understand how Spring handle files modification better.

jonleighton referenced this pull request Oct 11, 2013
Pre-1.0 support was broken anyway, and I don't want to maintain it.
@thibaudgg
Copy link
Author

There's still one failure with Celluloid (https://travis-ci.org/jonleighton/spring/jobs/12520312) but I'm not sure how to fix it and why it happens.

@tarcieri have you already such mailbox deadlock error, is it possible that it comes from travis-ci?

Backtrace:

/home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/celluloid-0.15.2/lib/celluloid/mailbox.rb:100:in `lock': deadlock detected (fatal)
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/celluloid-0.15.2/lib/celluloid/mailbox.rb:100:in `shutdown'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/celluloid-0.15.2/lib/celluloid/actor.rb:124:in `kill'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/celluloid-0.15.2/lib/celluloid.rb:171:in `block in shutdown'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/celluloid-0.15.2/lib/celluloid.rb:169:in `each'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/celluloid-0.15.2/lib/celluloid.rb:169:in `rescue in shutdown'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/celluloid-0.15.2/lib/celluloid.rb:177:in `shutdown'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/celluloid-0.15.2/lib/celluloid.rb:131:in `block in register_shutdown'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application.rb:110:in `fork'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application.rb:110:in `serve'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application.rb:81:in `block in run'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application.rb:71:in `loop'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application.rb:71:in `run'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application_manager.rb:103:in `block in start_child'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application_manager.rb:90:in `fork'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application_manager.rb:90:in `start_child'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application_manager.rb:30:in `start'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application_manager.rb:56:in `block in with_child'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application_manager.rb:24:in `synchronize'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application_manager.rb:43:in `with_child'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/application_manager.rb:64:in `run'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/server.rb:69:in `serve'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/server.rb:54:in `block in start_server'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/server.rb:54:in `loop'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/server.rb:54:in `start_server'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/server.rb:45:in `boot'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/server.rb:20:in `boot'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/client/run.rb:36:in `block in boot_server'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/client/run.rb:34:in `fork'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/client/run.rb:34:in `boot_server'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/client/run.rb:18:in `call'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/client/command.rb:7:in `call'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/client/rails.rb:23:in `call'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/client/command.rb:7:in `call'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/lib/spring/client.rb:23:in `run'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/gems/spring-0.0.11/bin/spring:6:in `<top (required)>'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/bin/spring:23:in `load'
    from /home/travis/build/jonleighton/spring/test/apps/rails-3-2/vendor/gems/1.9.3/bin/spring:23:in `<main>'```

@tarcieri
Copy link

@thibaudgg it's possible this is an actual race in the shutdown code. #kill is known to be racy

cc @halorgium

@thibaudgg
Copy link
Author

It does the same even with Celluloid.shutdown commented.

@tarcieri
Copy link

Are you using celluloid/test? Otherwise Celluloid will automatically install an at_exit handler that calls shutdown

@thibaudgg
Copy link
Author

Mmm same issue when using celluloid/test.

There's a fork in Spring application.rb in the backtrace, maybe it comes from there? https://travis-ci.org/jonleighton/spring/jobs/12630225#L84

@tarcieri
Copy link

Make sure that celluloid/test is required first before anything else requires celluloid or celluloid/autostart. It still looks like the at_exit handler is being called

thibaudgg referenced this pull request in guard/listen Oct 16, 2013
Work in progress (doesn't work yet)
@thibaudgg
Copy link
Author

It seems to me that it's required well before anything: https://github.com/guard/spring/blob/ca48d23e43227b679ec8f168e1ec05f3dd39ab56/test/helper.rb#L7-L8

@tarcieri
Copy link

Interesting! @halorgium, any idea what might be happening here?

@halorgium
Copy link

I will have a look when I'm back in town.

@thibaudgg
Copy link
Author

@halorgium Thanks in advance!

@halorgium
Copy link

@thibaudgg having some trouble getting the tests passing with master.
https://gist.github.com/halorgium/6d00c2ea8baccf4f4003

In terms of this change, it seems that listen should have a way of supporting the start/stop of itself.
This is related to some of the issues we're discussing in celluloid/dcell#51.

@thibaudgg
Copy link
Author

@halorgium on which system are you? [Listen warning]: Listen will be polling for changes. should never happens on OS X or Linux.

In terms of this change, it seems that listen should have a way of supporting the start/stop of itself.

I'm not sure to understand, Listen have a start/stop method (used by Spring).

@halorgium
Copy link

@thibaudgg I'm on Mac OS X. I would expected it to use fsevent, yup.

Just like Celluloid abstracts away the starting of our internal thread pool, I was suggesting that Listen could do the same thing so a user might do something like this:

require 'listen/test'

RSpec.configure do |config|
  config.before(:each) do |ex|
    Listen.stop
    Listen.start
  end
end

@tarcieri and I have not totally figured out if libraries should present Celluloid concepts to their users.
Or perhaps if we make something like erlang applications making Celluloid appear more like Rack to users.

@thibaudgg
Copy link
Author

@halorgium Yep, it should definitely work on OS X, what is your target_os in ruby? (Maybe I need to require rbconfig).

Ok I understand, I think that Listen.start would not be possible as all is started by Listen.to(...) that return a listener. But Listen.stop to ensure that all actors are terminated seems a good idea, I'll give it a try.

@tarcieri @halorgium I was asking myself the same question, now Listen only return a listener object that is not a Celluloid actor, I was thinking about changing that (maybe when the Signal trapping would be implemented) and use the Celluloid concepts to starting it (.async.start or .start). Is there an github issue to follow about it?

@tarcieri
Copy link

@thibaudgg that sounds good to me

@jmuheim
Copy link

jmuheim commented Dec 4, 2013

I'd like to use this un-merged branch, but I don't know how to target it in my Gemfile. Thanks for help! 👍

@senny
Copy link
Member

senny commented Dec 4, 2013

@jmuheim use:

gem "spring", github: "guard/spring", branch: "listen2"

@jmuheim
Copy link

jmuheim commented Dec 4, 2013

This results in:

Bundler could not find compatible versions for gem "spring":
  In Gemfile:
    spring-commands-rspec (>= 0) ruby depends on
      spring (>= 0.9.1) ruby

    spring (0.0.11)

Gemfile.lock:

spring-commands-rspec (1.0.0)

@jmuheim
Copy link

jmuheim commented Dec 4, 2013

I did an gem update spring-commands-rspec, and now it's working. Thanks!

@nathany
Copy link

nathany commented Dec 6, 2013

👍 listen 2

@jeroenj
Copy link

jeroenj commented Aug 27, 2014

Is there any news on this and/or are there plans to continue work on this?

For now I'll disable listen, but it would be great to be able to use it again. :)

@pftg
Copy link

pftg commented Sep 8, 2014

👍 will be great to see this PR merged. In order of specific of one of my project I have more then 6 envs, and even for SSD spring use a lot of resource to polling files' changes.

@SFEley
Copy link

SFEley commented Oct 22, 2014

👍 here too. Is there still a problem with this branch? Anything we can do to help?

@thibaudgg
Copy link
Author

@e2 maybe you have an idea and a better understanding on that one! Would be awesome and really helpful, thanks!

@e2
Copy link

e2 commented Oct 22, 2014

@thibaudgg - I'll see what I can do.

Travis failures - couldn't reproduce locally (unit tests work without issues on my setup for rails 4.0.0) - Although I'm guessing record building has become quite fast in Listen, so the project tree might have been causing a slowdown (lots of files dues to gems in /vendor dir).

Acceptance tests needed a few tweaks - basically bundler was using installed gems and not installing them into vendor/gems (and the tests were failing due to rake gem not present - just like in your case).

I'm not too sure about the listen gem activation problem, but I think I can work it out.

I'll get the listen2 branch working, then I'll try merging with master, then testing, then rebase and at that point I could catch up and review what really needs for listen 2x to happen in spring (I'm allergic to polling too...).

@thibaudgg
Copy link
Author

@e2 awesome, thanks! 😍

@e2
Copy link

e2 commented Oct 24, 2014

All tests are green, and they should now also work locally without needing CI=1.

Feedback is welcome (e.g. most changes are to fix the tests or make them easier to work with - so I could create a separate PR if necessary, etc.).

@thibaudgg
Copy link
Author

@e2 great work, thanks!

@jonleighton @senny finally ready to be merged!

@pftg
Copy link

pftg commented Oct 24, 2014

@e2 will check your PR on real app ;)

@pjg
Copy link

pjg commented Oct 26, 2014

@e2 That is one awesome piece of work! Thank you for doing that! I'm looking forward to seeing it being merged!

@jonleighton
Copy link
Member

Hi guys,

Thanks to everyone who has chipped in here, and sorry for the delay in replying.

I have had in my head for a while that I'd like to extract the listen code into a separate gem.

This would enable it to be maintained by separate maintainers and outside of the release cycle of spring itself. I personally do not use the listen watcher, and so I'm keen for it to be maintained by people who do. I also want to minimise the amount of code which needs to exist in the spring gem.

To that end, I've extracted the existing code here: https://github.com/jonleighton/spring-watcher-listen

There's a supporting spring branch here: https://github.com/rails/spring/tree/extract_listen

What I suggest is the following:

  • I merge that branch and release spring 1.2.0 and spring-watcher-listen 1.0.0, which will provide the same functionality as we have already
  • People volunteer to maintain spring-watcher-listen (please shout if you're willing)
  • Those people then port this patch over to the spring-watcher-listen gem and release a new version of their own schedule (it shouldn't be hard as the structure of the code has not changed)

How does this sound? Who will volunteer to be a maintainer?

Thanks

@e2
Copy link

e2 commented Nov 15, 2014

I personally do not use the listen watcher

I personally don't use use spring, but I maintain and develop listen anyway.

I'd probably drop support for Listen 1.0 though (it's so ancient, using it doesn't make sense) - so I'm 👍 for extracting spring-watcher-listen into a separate gem.

So, sign me up for now.

@jonleighton
Copy link
Member

I'd probably drop support for Listen 1.0 though (it's so ancient, using it doesn't make sense) - so I'm 👍 for extracting spring-watcher-listen into a separate gem.

Yep, sounds good. I only ported the listen 1 code into the new project as that's what's in spring at the moment. But upgrading it to listen 2 and releasing that seems good to me.

@jonleighton
Copy link
Member

Ok, I've just released Spring 1.2.0, which includes the changes which extract Listen to the spring-watcher-listen gem. I've also released version 1.0.0 of that gem, which supports Listen 1.0, purely because it would be weird to have the Spring README recommend installing that gem, and then have that gem not exist while Listen 2 support is being worked on.

@e2 is now a collaborator on the spring-watcher-listen repo, and a gem owner on rubygems.org. He can release the Listen 2 support according to his own timetable as it's not longer dependent on the Spring project itself. Of course, if any changes need to be made in the Spring project to allow spring-watcher-listen to hook in appropriately, then I'm fully supportive of that.

I'm closing this PR now. Please continue the discussion at rails/spring-watcher-listen#1. Thanks!

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

Successfully merging this pull request may close these issues.

None yet