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

Commas in directory names causes hang in ActiveSupport::FileUpdateChecker #5445

Closed
wants to merge 1 commit into from

Conversation

JonRowe
Copy link
Contributor

@JonRowe JonRowe commented Mar 15, 2012

Having commas in directory names passed through to ActiveSupport::FileUpdateChecker causes an infinite hang whilst Dir[@glob] tries and fails to list files in these directories. This wasn't an issue before ~> 3.2 but on 3.2 upwards it prevents Rails from booting in development mode and prevents test suites running (rspec, cucumber at least).

Commas are perfectly valid parts of directory names (on OS X at least) and escaping them when compiling the glob solves this issue.

This issue can be triggered by adding such directories to the autoload component of rails config see: https://github.com/JonRowe/rails-broken-dir-example for an example broken application.

@josevalim
Copy link
Contributor

Thanks for the fix. Could you please:

  1. fix the pull request so it does not rely on fork since tests need to pass on windows as well
  2. send a pull request targeting the master branch

@JonRowe
Copy link
Contributor Author

JonRowe commented Mar 15, 2012

Wow, quick response, I will do so, do you have a suggestion on a way to do this without fork? Or is the test suite freezing preferable for windows compatible? (only on fail)

@pixeltrix
Copy link
Contributor

Shouldn't all the Dir.glob characters be escaped?

@JonRowe
Copy link
Contributor Author

JonRowe commented Mar 15, 2012

Probably, I have only proved that comma's trigger this issue though.

@josevalim
Copy link
Contributor

Jon, maybe you can span a thread? Timeout should work fine with threads.

@JonRowe
Copy link
Contributor Author

JonRowe commented Mar 15, 2012

As requested, now based off master #5448

@JonRowe
Copy link
Contributor Author

JonRowe commented Mar 15, 2012

Moving discussion to #5448. Note that the fork code in this test patch is actually flawed anyway (no cleanup of stuck processes).

@JonRowe JonRowe closed this Mar 15, 2012
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

3 participants