Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

Projects
None yet
3 participants
Contributor

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.

Owner

JonRowe commented on c334022 Mar 15, 2012

Apologies for the slight massiveness of the test but the Dir[] hang prevents timeout from firing too. So in order to produce a failing test this slightly overkill setup was needed.

Contributor

josevalim commented Mar 15, 2012

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
Contributor

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)

Owner

pixeltrix commented Mar 15, 2012

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

Contributor

JonRowe commented Mar 15, 2012

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

Contributor

josevalim commented Mar 15, 2012

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

Contributor

JonRowe commented Mar 15, 2012

As requested, now based off master #5448

Contributor

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