TCP forwarding #139

timkurvers · 2013-09-23T19:11:10Z

Initial implementation for broadcasting and receiving file system modifications over TCP, as discussed in #57.

timkurvers · 2013-09-23T19:18:18Z

The Celluloid::IO::TCPSocket connectivity issues turned out to be Celluloid internally using a DNS-resolver, which would not function correctly if the broadcaster had been set to listen to localhost. Unless explicitly specified the broadcaster now listens on all addresses by default.

thibaudgg · 2013-09-25T07:28:05Z

lib/listen.rb

+      TCP::Listener.new(target, :broadcaster, *args, &block)
+    else
+      Listener.new(*args, &block)
+    end


What do you think of using another method for forwarding, maybe something like Listen.for(target, 'dir_path') or Listen.to_forward...

I don't really like this big if in the to method, or maybe move it to another _listener_class private method?

From an aesthetic perspective I like how Listen.to 'foo', forward_to: '10.0.0.1' flows, as well as Listen.on '10.0.0.1:4000', but I agree on the current implementation being rather hacky and far from ideal.

What would the _listener_class private method you had in mind look like?

Mmm after some more thinking and because how TCP::Listener is different to initialize than Listener, _listener_class idea seems not so good anymore. :(

I agree than Listen.to('foo', forward_to: '10.0.0.1') is quite straightforward. So don't touch it for now. Thanks!

thibaudgg · 2013-09-25T07:34:37Z

Please rebase from master, I have fixed the specs on Travis. :)

thibaudgg · 2013-09-25T07:36:48Z

Overall I really like the coding style, thanks for this awesome PR. CodeClimate will like it!

I'll give it a try in the next days.

thibaudgg · 2013-09-26T18:58:21Z

I have test it locally with:

broadcaster.rb:

require 'listen'
listener = Listen.to(Dir.pwd, forward_to: '127.0.0.1:4000')
listener.start
sleep

receiver.rb

require 'listen'
listener = Listen.on('127.0.0.1:4000') do |modified, added, removed|
  puts "-----------"
  puts "modified: #{modified}"
  puts "added:    #{added}"
  puts "removed:  #{removed}"
end
listener.start
sleep

A problem is that first modification is always detected as an addition because receiver.rb isn't able to build a record on start. The solution is to give the responsibility of change type (modified, added or removed) detection on the broadcaster side and sent change with it. Makes sense?

coveralls · 2013-09-28T18:00:52Z

Changes Unknown when pulling a0afe4f on timkurvers:feature/tcp-forwarding into * on guard:master*.

coveralls · 2013-09-28T18:30:43Z

Changes Unknown when pulling e3a8a97 on timkurvers:feature/tcp-forwarding into * on guard:master*.

timkurvers · 2013-09-28T19:06:23Z

@thibaudgg I seem to get the first modification detected as addition even on master or on the TCP broadcaster, not just the TCP recipient. Can you confirm that?

Regardless, your assessment of the TCP receiver not handling this correctly is accurate, so I'll look into that.

Any idea what is up with the Changes Unknown when pulling.. from Coveralls/Travis?

coveralls · 2013-09-28T20:15:21Z

Changes Unknown when pulling f5d4328 on timkurvers:feature/tcp-forwarding into * on guard:master*.

coveralls · 2013-09-28T20:39:36Z

Changes Unknown when pulling 00d0c77 on timkurvers:feature/tcp-forwarding into * on guard:master*.

thibaudgg · 2013-10-01T06:33:08Z

@timkurvers for me, on OS X with darwin and polling adapter, first detection is detected as modification. On which platform are you?

It seems you manage to fix Changes Unknown when pulling.. right? Thanks!

require 'listen'
listener = Listen.to(Dir.pwd) do |modified, added, removed|
  puts "-----------"
  puts "modified: #{modified}"
  puts "added:    #{added}"
  puts "removed:  #{removed}"
end
listener.start
sleep

timkurvers · 2013-10-01T17:11:39Z

On OSX here as well. When listening to directories that are not the current working directory I seem to get the issue:

require 'listen'
listener = Listen.to 'lib' do |*args|
  p args
end
listener.start
sleep

$ touch lib/listen.rb

[[], ["/listen/lib/listen/adapter/windows.rb", "/listen/lib/listen/adapter/polling.rb",
...
"/listen/lib/listen/adapter.rb", "/listen/lib/listen.rb"], []]

With the polling adapter however, that does not occur:

require 'listen'
listener = Listen.to 'lib', force_polling: true do |*args|
  p args
end
listener.start
sleep

$ touch lib/listen.rb

[["lib/listen.rb"], [], []]

The Changes Unknown when pulling-message is still showing up for me here, not sure what's up. I did manage to fix the build, that turned out to be an encoding oversight.

thibaudgg · 2013-10-01T18:33:43Z

Mmm, I'm not able to reproduce it even using an external directory, what kind of hard drive do you have (SSD here)? Please try to wait some time before touching the file to see if it change anything.

Where are you seeing The Changes Unknown when pulling... message? In travis? Could you give an url please?

timkurvers · 2013-10-01T19:51:29Z

On an SSD here, too. I tried waiting a bit but that didn't change anything. I'm running the examples from within bundle exec pry, is that an issue?

The Changes Unknown when pulling-message are here in this thread by Coveralls, a couple of posts above.

thibaudgg · 2013-10-01T19:53:42Z

I launch it from a ruby file with bundle exec ruby ./example.rb, could you try it as well? Thanks.

Ok I see the errors message, I think we can skip it for now...

timkurvers · 2013-10-02T16:23:20Z

Have tried it with the following bundle exec ruby example.rb, containing:

#!/usr/bin/env ruby
require 'listen'

listener = Listen.to 'lib', 'spec', debug: true do |*args|
  p args
end

listener.start
sleep

The first file added, modified or removed in each of those directories seem to fire of additions. Anything that would help debugging this? Gemfile.lock?

thibaudgg · 2013-10-02T19:01:20Z

Ok nice, I was able to reproduce it. It's only happen when directory aren't given with absolute path. I'll fix it, thanks!

thibaudgg · 2013-10-02T19:14:35Z

Fixed in 146b5cd, could you rebase and give it a try please? Thanks!

coveralls · 2013-10-05T13:55:53Z

Coverage increased (+7.15%) when pulling 45ad9d8 on timkurvers:feature/tcp-forwarding into 5f0c9b7 on guard:master.

timkurvers · 2013-10-05T13:56:43Z

Excellent, works fine! I can also confirm the TCP receiver correctly relies on the broadcasted changes.

thibaudgg · 2013-10-05T15:01:33Z

Great news, I just released Listen 2.0.0, so this feature will be added in 2.1.0.
You just need to add some more specs and it will be merge-able right?

timkurvers · 2013-10-07T13:23:04Z

Yep! I will most probably need some help with speccing threaded/Celluloid'd code though.

thibaudgg · 2013-10-07T13:31:16Z

Sure, feel free to ask for help.

ghost · 2013-10-28T21:13:09Z

Nice work. Cant wait to see it merged!

ghost · 2013-11-15T18:41:04Z

If someone wants to actually try this here's a simple way to build it into guard: https://github.com/johannesbecker/guard/commit/a9c14b514ffd4c64b97ffd8188e5f197f095ada1

Additionally you need to rebase this PR on Listen 2.2 - did so at https://github.com/johannesbecker/listen

Works like a charm 👍

timkurvers · 2013-11-15T20:13:26Z

Thanks for giving this a go @JohannesBecker, much appreciated! I have gotten a bit side-tracked with other projects but hope to be able to spec the PR soonish.

thibaudgg · 2013-11-18T08:57:36Z

Great can't wait to merge it!

thibaudgg · 2013-11-18T08:58:40Z

@JohannesBecker once merged, it would be great if you could submit your branch to Guard via a PR, thanks!

timkurvers · 2014-02-09T21:02:48Z

The message implementation now uses JSON.

Wasn't really sure where to document TCP forwarding in the README but I felt it was much less of a core feature than the adapters, so it's beneath those. If anything needs to be amended please let me know :)

Add TCP forwarding feature

thibaudgg · 2014-02-11T08:01:26Z

Awesome, @timkurvers thanks a lot for your great work! 2.5.0 released!

acoulton · 2014-02-11T08:37:13Z

@timkurvers @thibaudgg thank you for this, it's going to be a major improvement to our workflow.

timkurvers · 2014-02-12T09:20:19Z

Wooo, thanks for merging!

ideasasylum · 2014-02-12T16:18:44Z

Thank you. Thank you! I had taken a stab at this almost a year ago and it defeated me in the time I had. Kudos!

So, can we use this with guard yet? Well, I've taken a stab at that and got it working really well:

https://github.com/ideasasylum/guard#-o--listen_on-option

I think I need a better host 'listener' script though as it can't be killed from the console

thibaudgg · 2014-02-12T16:20:59Z

@ideasasylum great, please submit a PR on Guard. I would be greatly happy to merge it!

ideasasylum · 2014-02-12T16:30:47Z

@thibaudgg done: guard/guard#555

thibaudgg · 2014-02-12T16:42:27Z

@ideasasylum thanks we'll continue the discussion there. (Tomorrow I think)

thibaudgg reviewed Sep 25, 2013
View reviewed changes

jhogendorn mentioned this pull request Dec 12, 2013

Listen does not catch file changes over Vagrants vboxfs jekyll/jekyll#1808

Closed

timkurvers added 19 commits February 8, 2014 23:24

Correct instance-method reference in comment

cb4a2a1

No need to manually terminate the broadcaster

952de2f

Drop private methods

1694b48

Drop infinite loops to allow stubbing in specs

1f1872f

Increase testability for TCP adapter

a856ea3

Correct class method descriptions

f5b2dd6

Normalize subject declarations

018fdd9

Register broadcaster in Celluloid registry

0b839bc

Force TCP adapter through options as listener is no longer received

d089cd2

Add Broadcaster#start to simplify testing

0e4a3be

Finalize specs for TCP listener, broadcaster and adapter

90addbe

Small comment corrections

71ba7cd

Normalize hosts

a59c065

Add acceptance spec for TCP forwarding

f7144b7

Drop Marshal in favour of JSON to avoid arbitrary code execution

db13d97

Restructure TCP acceptance spec

3fae434

Honour stopped state

d4554af

Broaden acceptance specs

aa1aee4

Document TCP forwarding in README

db59b4a

acoulton mentioned this pull request Feb 10, 2014

Document difference with Vagrant docs winnfsd/vagrant-winnfsd#7

Closed

thibaudgg added a commit that referenced this pull request Feb 11, 2014

Merge pull request #139 from timkurvers/feature/tcp-forwarding

9be4f2d

Add TCP forwarding feature

thibaudgg merged commit 9be4f2d into guard:master Feb 11, 2014

TCP forwarding #139

TCP forwarding #139

Conversation

timkurvers commented Sep 23, 2013

timkurvers commented Sep 23, 2013

thibaudgg Sep 25, 2013

Choose a reason for hiding this comment

timkurvers Sep 25, 2013

Choose a reason for hiding this comment

thibaudgg Sep 26, 2013

Choose a reason for hiding this comment

thibaudgg commented Sep 25, 2013

thibaudgg commented Sep 25, 2013

thibaudgg commented Sep 26, 2013

coveralls commented Sep 28, 2013

coveralls commented Sep 28, 2013

timkurvers commented Sep 28, 2013

coveralls commented Sep 28, 2013

coveralls commented Sep 28, 2013

thibaudgg commented Oct 1, 2013

timkurvers commented Oct 1, 2013

thibaudgg commented Oct 1, 2013

timkurvers commented Oct 1, 2013

thibaudgg commented Oct 1, 2013

timkurvers commented Oct 2, 2013

thibaudgg commented Oct 2, 2013

thibaudgg commented Oct 2, 2013

coveralls commented Oct 5, 2013

timkurvers commented Oct 5, 2013

thibaudgg commented Oct 5, 2013

timkurvers commented Oct 7, 2013

thibaudgg commented Oct 7, 2013

ghost commented Oct 28, 2013

ghost commented Nov 15, 2013

timkurvers commented Nov 15, 2013

thibaudgg commented Nov 18, 2013

thibaudgg commented Nov 18, 2013

timkurvers commented Feb 9, 2014

thibaudgg commented Feb 11, 2014

acoulton commented Feb 11, 2014

timkurvers commented Feb 12, 2014

ideasasylum commented Feb 12, 2014

thibaudgg commented Feb 12, 2014

ideasasylum commented Feb 12, 2014

thibaudgg commented Feb 12, 2014