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: Ensure theme JavaScript cache get consistent SHA1 digest (stable backport) #16669

Merged
merged 1 commit into from
May 6, 2022

Conversation

davidtaylorhq
Copy link
Member

@davidtaylorhq davidtaylorhq commented May 6, 2022

(Stable backport of 7ed899f)

There is a couple of layers of caching for theme JavaScript in Discourse:

The first layer is the javascript_caches table in the database. When a theme
with JavaScript files is installed, Discourse stores each one of the JavaScript
files in the theme_fields table, and then concatenates the files, compiles
them, computes a SHA1 digest of the compiled JavaScript and store the results
along with the SHA1 digest in the javascript_caches table.

Now when a request comes in, we need to render <script> tags for the
activated theme(s) of the site. To do this, we retrieve the javascript_caches
records of the activated themes and generate a <script> tag for each record.
The src attribute of these tags is a path to the /theme-javascripts/:digest
route which simply responds with the compiled JavaScript that has the requested
digest.

The second layer is a distributed cache whose purpose is to make rendering
<script> a lot more efficient. Without this cache, we'd have to query the
javascript_caches table to retrieve the SHA1 digests for every single
request. So we use this cache to store the <script> tags themselves so that
we only have to retrieve the javascript_caches records of the activated
themes for the first request and future requests simply get the cached
<script> tags.

What this commit does it ensures that the SHA1 digest in the
javascript_caches table stay the same across compilations by adding an order
by id clause to the query that loads the theme_fields records. Currently, we
specify no order when retrieving the theme_fields records so the order in
which they're retrieved can change across compilations and therefore cause the
SHA1 to change even though the individual records have not changed at all.

An inconsistent SHA1 digest across compilations can cause the database cache
and the distributed cache to have different digests and that causes the
JavaScript to fail to load (and if the theme heavily customizes the site, it
gives the impression that the site is broken) until the cache is cleared.

This can happen in busy sites when 2 concurrent requests recompile the
JavaScript files of a theme at the same time (this can happen when deploying a
new Discourse version) and request A updates the database cache after request B
did, and request B updates the distributed cache after request A did.

Internal ticket: t60783.

Co-authored-by: David Taylor david@taylorhq.com

There is a couple of layers of caching for theme JavaScript in Discourse:

The first layer is the `javascript_caches` table in the database. When a theme
with JavaScript files is installed, Discourse stores each one of the JavaScript
files in the `theme_fields` table, and then concatenates the files, compiles
them, computes a SHA1 digest of the compiled JavaScript and store the results
along with the SHA1 digest in the `javascript_caches` table.

Now when a request comes in, we need to render `<script>` tags for the
activated theme(s) of the site. To do this, we retrieve the `javascript_caches`
records of the activated themes and generate a `<script>` tag for each record.
The `src` attribute of these tags is a path to the `/theme-javascripts/:digest`
route which simply responds with the compiled JavaScript that has the requested
digest.

The second layer is a distributed cache whose purpose is to make rendering
`<script>` a lot more efficient. Without this cache, we'd have to query the
`javascript_caches` table to retrieve the SHA1 digests for every single
request. So we use this cache to store the `<script>` tags themselves so that
we only have to retrieve the `javascript_caches` records of the activated
themes for the first request and future requests simply get the cached
`<script>` tags.

What this commit does it ensures that the SHA1 digest in the
`javascript_caches` table stay the same across compilations by adding an order
by id clause to the query that loads the `theme_fields` records. Currently, we
specify no order when retrieving the `theme_fields` records so the order in
which they're retrieved can change across compilations and therefore cause the
SHA1 to change even though the individual records have not changed at all.

An inconsistent SHA1 digest across compilations can cause the database cache
and the distributed cache to have different digests and that causes the
JavaScript to fail to load (and if the theme heavily customizes the site, it
gives the impression that the site is broken) until the cache is cleared.

This can happen in busy sites when 2 concurrent requests recompile the
JavaScript files of a theme at the same time (this can happen when deploying a
new Discourse version) and request A updates the database cache after request B
did, and request B updates the distributed cache after request A did.

Internal ticket: t60783.

Co-authored-by: David Taylor <david@taylorhq.com>
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/missing-theme-javascripts-assets-http-404/226124/4

@CvX
Copy link
Contributor

CvX commented May 6, 2022

Co-authored-by: David Taylor david@taylorhq.com

😂

@davidtaylorhq
Copy link
Member Author

🤣 (For the record, that co-authored-by line is from Osama's original commit - I didn't add it 😛)

@davidtaylorhq davidtaylorhq merged commit a0c141d into stable May 6, 2022
@davidtaylorhq davidtaylorhq deleted the stable-theme-sha1 branch May 6, 2022 10:10
@oblakeerickson oblakeerickson mentioned this pull request Jun 13, 2022
@xfalcox xfalcox mentioned this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants