Fixes #root race condition #449

Merged
merged 1 commit into from Sep 14, 2011

Projects

None yet

3 participants

@jcarlson
Contributor
jcarlson commented Sep 6, 2011

When CarrierWave is loaded, the Configuration module is loaded prior to setting CarrierWave.root. At the time the configuration 'defaults' are established, CarrierWave.root is nil, causing Uploader instances to initially carry a #root value of nil, unless config.root is explicitly configured either globally or on the Uploader instance.

This condition was also difficult to test for (and in fact an existing test for this was hiding the error). During test setup, in 'spec_helper.rb', a value is explicitly set for CarrierWave.root before any of the modules (including Configuration) are loaded. This causes the test in 'paths_spec.rb' to give false positives.

In normal operation, the Configuration module loads before CarrierWave.root is set, near the end of carrierwave.rb.

Rather than re-order the initialization process and module loading, I changed the Configuration module to accept Proc values for configuration values. This has the added bonus of allowing configuration values to be dynamically evaluated at runtime, rather than at startup.

I have set the default value of config.root as a Proc to resolve the race condition.

Additionally, I have corrected the 'paths_spec.rb' unit test to ensure the existing test fails, exposing the error, and a new test has been added to ensure that the Proc execution resolves correctly on the uploader instance.

@trevorturk trevorturk merged commit 9c676d8 into carrierwaveuploader:master Sep 14, 2011
Contributor
Contributor

The pull request was based off of stable code v0.5.7. All tests passed with the exception of one test I left failing to demonstrate the error.

I will remove the failing test, since it was not properly testing the condition for which it was intended to test.

If you'll reopen the pull request, I will submit my changes shortly.

Contributor

Please just provide a new pull request if that's OK with you

Owner
bensie commented Sep 14, 2011

He didn't break the build -- it's been broken on JRuby for awhile now. You just haven't gotten the emails until now :)

Contributor

Resubmitted pull request as #458

Contributor

No, I think there was a failure: http://travis-ci.org/#!/jnicklas/carrierwave/builds/157662

I think I tested locally, too. Sorry if I made a mistake, though. I was working late ;(

Owner
bensie commented Sep 14, 2011

Oh ok -- you're right. Cool if we remove JRuby for now? I'm not sure how to fix and it would be nice to get accurate emails for build status.

Contributor

yeah please do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment