Concurrency problem creating assets directory

[doug-fish]

In OpenStack Horizon we are seeing this error intermittently during automated testing:

File exists: '/opt/stack/new/horizon/static/scss/assets' http://logs.openstack.org/81/120281/3/check/check-tempest-dsvm-neutron-full/fbe5341/logs/screen-horizon.txt.gz

It looks like the problem is in this line https://github.com/fusionbox/django-pyscss/blob/master/django_pyscss/scss.py#L161

It would be great if that code didn't throw the File Exists error if the path was created successfully by another thread/process.

[coveralls]

Coverage decreased (-0.34%) when pulling fb1fb223d91b40da18a53716b1dc1c2db9dc7c53 on double_create_assets into 138310db10995ee33c4e114d0bcab77d8deb21a5 on master.

[rockymeza]

Hi @doug-fish,

Thanks for reporting this error. I have written a patch to fix it. Could you please test out the double_create_assets branch to see if it fixes your problem?

@doug-fish, @deshipu I also wanted to ask you if you think it should be in the purvey of DjangoScss to create that directory? I put the code in there because I need something to create the ASSETS_ROOT so that I wouldn't forget, but I don't think it will work with all storage backends not to mention it seems to be a little unexpected. What do you think?

I attempted to write a test to reproduce it, but I don't think the test is right anymore. I'm not sure how to reproduce the race condition in the test. Any thoughts?

cc @gavinwahl.

[doug-fish]

@rockymeza thanks for putting together this fix so quickly!

Testing this failure has been a problem for me as well. The automated testing that if failing is running large numbers of tests in parallel. I assume the storage response time must be very slow in order to get this failure! I haven't been able to reproduce the error in my dev environment; I don't have any suggestions for writing a good test case. I can say, based on inspection of the code you've written, I believe it will solve the problem. I'd love to see us pick it up in Horizon at the next release.

Regarding the creation of the directory, I'm outside my expertise to suggest if that should be a responsibility of DjagnoScss or not. Maybe @deshipu has an opinion?

[deshipu]

Looking at the code, it's the classical "look before you jump" thing, that should be replaced with "ask for forgiveness, not permission" -- that is, it should simply attempt to create the directory and catch the exception if it already exists, without checking if it exists first.

On the other hand, there is the question whether it's the responsibility of Django-PyScss to create that directory or not. Honestly, I have no idea. In either case, it should report a meaningful error when the directory is missing, though (either because it wasn't created, or because creation failed for other reason than it already existing).

[rockymeza]

@gavinwahl, it already stopped testing anything because I changed the implementation of idempotent_makedirs. Are you cool with just not having a test for this?