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

do not treat non-existent paths as dirs to prevent further scanning [fix #354] #357

Closed
wants to merge 2 commits into from
Closed

do not treat non-existent paths as dirs to prevent further scanning [fix #354] #357

wants to merge 2 commits into from

Conversation

unjello
Copy link

@unjello unjello commented Nov 13, 2015

No description provided.

@e2
Copy link
Contributor

e2 commented Nov 14, 2015

I think I'd prefer a :none result here. (Especially if there's no test for it).

Even if it increases the branch complexity, at least it's more semantically correct and more intuitive to deal with later.

@unjello
Copy link
Author

unjello commented Nov 14, 2015

Agreed. Although to be honest it deserves a rewrite anyway. At least a minor one where you could infer the type from inspecting previous records. But that may be the next step. For starters - lets stop crashing :)

@e2
Copy link
Contributor

e2 commented Nov 14, 2015

Although to be honest it deserves a rewrite anyway.

The problem is finding time to do so. Listen 3.x was already a major rewrite (which happened not long ago).

At least a minor one where you could infer the type from inspecting previous records.

No, that approach actually failed. It's best to never assume what's on the filesystem may match what was recorded previously. Moving trees would just cause all kinds of problems.

For starters - lets stop crashing :)

That's not a good approach. Every problem has to be well understood and fixed properly. Because a crash is easy to fix, but debugging a logical problem can take ages - and it's often impossible to reproduce.

Crashes are good. The solution to crashes is understanding and prevention. (Otherwise I'd be digging myself a grave...)

You use case seems strange. Let's look at the code here (simplified for clarity):

      current = Set.new(Pathname('foo').children)
      current.each do |full_path|
        ::File.lstat(full_path.to_s)
      end

An Errno::ENOENT here suggest that the list of files (children) is no longer valid.

To me the solution is to handle Errno::ENOENT by rescanning the directory (because the old list obviously no longer reflects what's on the filesystem).

So I'd suggest something like this:

begin
  current = Set.new(Pathname('foo').children)
  current.each do |full_path|
    ::File.lstat(full_path.to_s)
    # (...)
  end
rescue Errno::ENOENT
  retry
end

(And so self.detect_type shouldn't handle the exception).

What do you think?

@e2
Copy link
Contributor

e2 commented Nov 14, 2015

Let me know if this solves the crashing problem: #358

@unjello
Copy link
Author

unjello commented Nov 18, 2015

Fixed by #358 and released in 3.0.5 with a different fix. Thanks :)

@unjello unjello closed this Nov 18, 2015
@unjello unjello deleted the fix_file_deletion branch November 18, 2015 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants