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

Fix offline manifest race condition #1086

Merged
merged 1 commit into from Dec 7, 2021
Merged

Fix offline manifest race condition #1086

merged 1 commit into from Dec 7, 2021

Conversation

cuu508
Copy link
Contributor

@cuu508 cuu508 commented Nov 26, 2021

django-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.

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.
Copy link
Contributor

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cuu508 — thanks for this.

Perhaps there's a way to avoid the lock entirely.

It looks OK to me. Maybe there's another approach but this seems simple enough to go with.

You're officially Hero of the Month here 🥇

@carltongibson carltongibson merged commit ef45175 into django-compressor:develop Dec 7, 2021
@cuu508 cuu508 deleted the fix_offline_manifest_race_condition branch December 13, 2021 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants