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

Crashes (OSX) on directories with colons in their names #361

Open
wisq opened this issue Jan 2, 2016 · 7 comments
Open

Crashes (OSX) on directories with colons in their names #361

wisq opened this issue Jan 2, 2016 · 7 comments
Labels

Comments

@wisq
Copy link

wisq commented Jan 2, 2016

Sample program:

require 'listen'
listener = Listen.to('/tmp/foo:bar') do |modified, added, removed|
  puts "modified absolute path: #{modified}"
  puts "added absolute path: #{added}"
  puts "removed absolute path: #{removed}"
end
listener.start # not blocking
sleep

If you touch files inside /tmp/foo:bar, listen will receive two events: one for /tmp/foo and one for bar. The latter causes an error due to being a relative path:

E, [2016-01-02T05:04:45.074518 #96993] ERROR -- : fsevent: running worker failed: different prefix: "" and "/private/tmp/foo:bar":/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/2.2.0/pathname.rb:508:in `relative_path_from'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/adapter/darwin.rb:44:in `block in _process_event'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/adapter/darwin.rb:40:in `each'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/adapter/darwin.rb:40:in `_process_event'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/adapter/base.rb:42:in `block (2 levels) in configure'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rb-fsevent-0.9.7/lib/rb-fsevent/fsevent.rb:45:in `call'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rb-fsevent-0.9.7/lib/rb-fsevent/fsevent.rb:45:in `run'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/adapter/darwin.rb:51:in `_run_worker'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/adapter/darwin.rb:35:in `_run'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/adapter/base.rb:78:in `block in start'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/internals/thread_pool.rb:6:in `call'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/internals/thread_pool.rb:6:in `block in add' called from: /Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/adapter/darwin.rb:50:in `_run_worker'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/adapter/darwin.rb:35:in `_run'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/adapter/base.rb:78:in `block in start'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/internals/thread_pool.rb:6:in `call'
/Users/wisq/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/listen-3.0.5/lib/listen/internals/thread_pool.rb:6:in `block in add'`

This is caused by guard/rb-fsevent#59, and I'm only reporting it here to let you know it's an ongoing issue, and/or in case you want to work around it until that bug is fixed.

@e2
Copy link
Contributor

e2 commented Jan 2, 2016

Thanks. I don't even use OSX myself, so I'll have to wait until it's fixed.

@gnclmorais
Copy link

Thank you so much for this bug report! Now I understand why this was not working…

@yez
Copy link

yez commented Mar 28, 2016

I started to look into this issue and have some interesting information to share.

I dug into the rb-fsevent to find the exact code responsible:

from: https://github.com/thibaudgg/rb-fsevent/blob/master/lib/rb-fsevent/fsevent.rb#L36

class FSEvent
  def run
    @pipe    = open_pipe
    @running = true

    # please note the use of IO::select() here, as it is used specifically to
    # preserve correct signal handling behavior in ruby 1.8.
    while @running && IO::select([@pipe], nil, nil, nil)
      if line = @pipe.readline
        modified_dir_paths = line.split(':').select { |dir| dir != "\n" }
        callback.call(modified_dir_paths)
      end
    end
  rescue Interrupt, IOError, Errno::EBADF
  ensure
    stop
  end
end

The important line here: modified_dir_paths = line.split(':').select { |dir| dir != "\n" } is the reason for the failure.

Working within the constraints of listen, what would a solution look like? Well I took a stab at altering the existing logic using a refinement:

module Listen
  module Adapter
    # Adapter implementation for Mac OS X `FSEvents`.
    #
    class Darwin < Base

      module FSEventRefinement
        refine ::FSEvent do
          def run
            @pipe    = open_pipe
            @running = true

            # please note the use of IO::select() here, as it is used specifically to
            # preserve correct signal handling behavior in ruby 1.8.
            while @running && IO::select([@pipe], nil, nil, nil)
              if line = @pipe.readline

                # First iterate through passed in list, searching for known ones
                known_dirs, unknown_dirs = line.split(':').partition { |s| Dir.exists?(s) }

                # Then iterate over the list of unknown directories, 
                #  combining them until they either make a known directory 
                #  or we run out of parts to combine
                i = 1
                while (unknown_dirs.length > 0 && i <= unknown_dirs.length)
                  attempt = unknown_dirs[0..i].join(':')
                  if Dir.exists?(attempt)
                    # Found one! Add it to the list of known and delete it from
                    #  the list of unknown
                    known_dirs << attempt
                    (0..i).each { |index| unknown_dirs.delete_at(index) }
                  else
                    i += 1
                  end
                end

                modified_dir_paths = known_dirs
                callback.call(modified_dir_paths)
              end
            end
          rescue Interrupt, IOError, Errno::EBADF
          ensure
            stop
          end
        end
      end

      using FSEventRefinement
    end
  end
end

So this code would solve the issue of directories being split too aggressively then doing nothing but it has other problems.

For instance, what happens when /tmp/foo is a directory that we don't want to listen to? This code will incorrectly add that directory to the listened set. To remedy this, the last step of this code could be to check that the input string matches exactly the same list of "known directories" this method produces. Sounds error-prone and tedious to me. I am sure there are other solutions but should they actually be used?

Also, a refinement solution is obviously not the way this would ultimately be addressed. It was used simply as an example.

Since OSX is backed by a UNIX based OS, it uses the : character as a delimiter when defining a list of directories.

This documentation from Apple and this StackOverflow answer both advise against using the : character in a file or folder on OSX.

Thoughts?

TL;DR - I don't think this should actually be fixed since naming folders with : is not advised in OSX

@e2
Copy link
Contributor

e2 commented Mar 29, 2016

I don't use OSX all (Linux only), so I can't test or maintain anything OSX-specific. (Any code that can be unit-tested on Linux is fine). This means I can't even compile the rb-fsevent watcher binary.

However ...

Notice you can supply a format to the fsevent_watch binary.

So it probably just needs to added to supported options here

You'll find that a different format may use a different separator

Any format except the default ("classic") should be much easier to work with.

(You can even just refine open_pipe to add the format option by force).

Again - I can't test this myself. But I hope it creates a more robust solution.

@wisq
Copy link
Author

wisq commented Mar 29, 2016

Travis CI has an OSX test environment, so I imagine it should be possible to have it test this one we come up with a solution.

I agree that using an alternate format would be the ideal solution. If we want a workaround before then, I think the best one would be, rather than checking the filesystem at all, we could just preprocess the list, look for any non-absolute paths, and combine them with the previous path. The only false negative would be if a directory ends with a colon (because /foo:/bar looks legit).

@e2
Copy link
Contributor

e2 commented Mar 29, 2016

@wisq -

Travis CI has an OSX test environment, so I imagine it should be possible to have it test this one we come up with a solution.

Yes, but that totally sucks for "debugging" when things aren't working. There are also edge cases you can't reasonably diagnose on OSX. E.g. I have no example of output with any format, so either I have create the sample output "in my head" based on the *.c files in rb-fsevent ... or I have to create a special PR where this is changed and debugging is enabled. And I can still get things wrong, because the integration tests are especially tricky - or many failures can occur due to timing and/or Travis being too busy. And if I do break anything, I'll just get lots of reports about stuff broken - without PRs.

(Example: once upon a time I broke OSX support due to how each watcher needed it's own thread).

I agree that using an alternate format would be the ideal solution. If we want a workaround before then, I think the best one would be, rather than checking the filesystem at all, we could just preprocess the list, look for any non-absolute paths, and combine them with the previous path.

That's too much work. Changing the format pretty much guarantees you'll see which files contain a ':' or not. And setting the format is simply not implemented on the Ruby side, so changing the format is trivial. The important part is: edge cases.

I'd suggest the following:

  1. Create a failing integration test (not hard - copy existing spec and create dirs with colons)
  2. Create a few fixtures based on the best format - and then provide unit tests for them

(I can help with 2 if I get some fixtures and someone creates at least a failing PR with 1).

@ColinDKelley
Copy link
Collaborator

ColinDKelley commented Sep 6, 2021

This issue is over 5 years old. Probably time to close it?!

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

No branches or pull requests

5 participants