Skip to content

Commit

Permalink
Fixed race condition in _compress_template (django-compressor#1086)
Browse files Browse the repository at this point in the history
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 django-compressor#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.
  • Loading branch information
cuu508 committed Dec 7, 2021
1 parent d4be504 commit ef45175
Showing 1 changed file with 12 additions and 2 deletions.
14 changes: 12 additions & 2 deletions compressor/management/commands/compress.py
Expand Up @@ -2,6 +2,7 @@
import os
import sys
import concurrent.futures
from threading import Lock

from collections import OrderedDict, defaultdict
from fnmatch import fnmatch
Expand All @@ -21,6 +22,7 @@
TemplateDoesNotExist)
from compressor.utils import get_mod_func

offline_manifest_lock = Lock()

class Command(BaseCommand):
help = "Compress content outside of the request/response cycle"
Expand Down Expand Up @@ -244,14 +246,22 @@ def _compress_template(offline_manifest, nodes, parser, template, errors):
rendered = parser.render_nodelist(template, context, node)
key = get_offline_hexdigest(rendered)

if key in offline_manifest:
continue
# Atomically check if the key exists in offline manifest.
# If it doesn't, set a placeholder key (None). This is to prevent
# concurrent _compress_template instances from rendering the
# same node, and then writing to the same file.
with offline_manifest_lock:
if key in offline_manifest:
continue

offline_manifest[key] = None

try:
result = parser.render_node(template, context, node)
except Exception as e:
errors.append(CommandError("An error occurred during rendering %s: "
"%s" % (template.template_name, smart_str(e))))
del offline_manifest[key]
return
result = result.replace(
settings.COMPRESS_URL, settings.COMPRESS_URL_PLACEHOLDER
Expand Down

0 comments on commit ef45175

Please sign in to comment.