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

Intermittent failure in offline compression tests. #1082

Closed
carltongibson opened this issue Nov 22, 2021 · 5 comments
Closed

Intermittent failure in offline compression tests. #1082

carltongibson opened this issue Nov 22, 2021 · 5 comments
Labels
Milestone

Comments

@carltongibson
Copy link
Contributor

ERROR: test_offline_django (compressor.tests.test_offline.OfflineCompressTestCaseWithContextVariableInheritance)

example: https://github.com/django-compressor/django-compressor/runs/4285761036?check_suite_focus=true#step:5:36

Doesn't happen every run.

@cuu508
Copy link
Contributor

cuu508 commented Nov 22, 2021

This might be an actual bug, not a broken test, but not sure.

The "compress" management command runs tasks using a threadpool and 4 workers. The OfflineCompressTestCaseWithContextVariableInheritance test case works with two templates:

base.html:

{% load compress %}
{% spaceless %}
    {% compress js %}
    <script type="text/javascript">
        alert("test using template extension passed in variable parent=base.html");
    </script>
    {% endcompress %}
{% endspaceless %}

and test_compressor_offline.html:

{% extends parent_template %}

Both templates have an identical {% compress js %} section, because one inherits from the other.

I instrumented the code to print the traceback for the underlying "No such file or directory" exception, here it is:

Traceback (most recent call last):
  File "/home/cepe/repos/django-compressor/compressor/management/commands/compress.py", line 251, in _compress_template
    result = parser.render_node(template, context, node)
  File "/home/cepe/repos/django-compressor/compressor/offline/django.py", line 117, in render_node
    return node.render(context, forced=True)
  File "/home/cepe/repos/django-compressor/compressor/templatetags/compress.py", line 152, in render
    return self.render_compressed(context, self.kind, self.mode, forced=forced, log=log, verbosity=verbosity)
  File "/home/cepe/repos/django-compressor/compressor/templatetags/compress.py", line 121, in render_compressed
    rendered_output = compressor.output(mode, forced=forced, basename=file_basename)
  File "/home/cepe/repos/django-compressor/compressor/js.py", line 47, in output
    ret.append(subnode.output(*args, **kwargs))
  File "/home/cepe/repos/django-compressor/compressor/js.py", line 49, in output
    return super().output(*args, **kwargs)
  File "/home/cepe/repos/django-compressor/compressor/base.py", line 330, in output
    return self.handle_output(mode, filtered_output, forced, basename)
  File "/home/cepe/repos/django-compressor/compressor/base.py", line 338, in handle_output
    return output_func(mode, content, forced, basename)
  File "/home/cepe/repos/django-compressor/compressor/base.py", line 351, in output_file
    self.storage.save(new_filepath, ContentFile(content.encode(self.charset)))
  File "/home/cepe/venvs/django-compressor/lib/python3.8/site-packages/django/core/files/storage.py", line 54, in save
    return self._save(name, content)
  File "/home/cepe/venvs/django-compressor/lib/python3.8/site-packages/django/core/files/storage.py", line 300, in _save
    os.chmod(full_path, self.file_permissions_mode)
FileNotFoundError: [Errno 2] No such file or directory: '/home/cepe/repos/django-compressor/compressor/tests/static/CACHE/js/output.b8376aad1357.js'

So, compressor calls CompressorFileStorage.save() concurrently. The CompressorFileStorage.get_available_name() also has this:

    def get_available_name(self, name, max_length=None):
        """
        Deletes the given file if it exists.
        """
        if self.exists(name):
            self.delete(name)
        return name

I'm guessing there's a race condition where concurrent threads save the same file, one thread deletes it and then the other thread tries to chmod it.

@carltongibson
Copy link
Contributor Author

Good work @cuu508 — that gives something to dig into 👍

Looks like #1060 was fixed upstream, so this should be the last blocker 🤞

@cuu508
Copy link
Contributor

cuu508 commented Nov 26, 2021

@carltongibson I'm cautiously optimistic this fixes the intermittent test failures: #1086

@carltongibson
Copy link
Contributor Author

Hey @cuu508 - thanks for this! I'll take a look now. 🥇

carltongibson pushed a commit that referenced this issue Dec 7, 2021
compressor avoids rendering the same node multiple times
by checking for a key in offline_manifest:

```
    if key in offline_manifest:
        continue

    [... render template ...]

    offline_manifest[key] = result
```

Multiple _compress_template calls can run concurrently,
creating a race condition: each checks for a key in
offline manifest, then each proceeds to render template.
Finally they try to save the same file at the same time,
causing #1082.

My proposed fix is to atomically
 * check if the manifest key exists
 * if it doesn't exist, set it to a placeholder value (None)

So, in nutshell, the first "if" part becomes:

```
    with offline_manifest_lock:
        if key in offline_manifest:
            continue

        offline_manifest[key] = None
```

I'm not sure where to store the lock, I've put it at the
module level currently. Perhaps there's a way to avoid
the lock entirely.
@carltongibson
Copy link
Contributor Author

Fixed by #1086. 👯 Well done @cuu508.

Next up a release... 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants