Skip to content

Commit

Permalink
Recognize text/javascript as valid js mimetype for caching purposes (#…
Browse files Browse the repository at this point in the history
…5746)

Recently the test core_plugin_test.py::test_js_cache began to fail in some environments with the following error:

```
... core_plugin_test.py", line 196, in test_js_cache
self.assertEqual(
AssertionError: 
- private, max-age=86400
+ no-cache, must-revalidate
```

The root cause is that these systems have now been configured to return 'text/javascript' as the mimetype for js files, where they were previously returning 'application/javascript'. This means the 'Cache-Control' header was not set by this line:

https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/core/core_plugin.py;l=137;drc=c061b659f50cde9dd9da5aa906ecb682ecdc68d4

```
        expires = (
            JS_CACHE_EXPIRATION_IN_SECS
            if request.args.get("_file_hash")
            and mimetype == "application/javascript"
            else 0
        )
```

After further investigation (thanks @nfelt!) we determined that the change in mimetype is likely intentional after the recent publishing of RFC 9239 (https://www.rfc-editor.org/rfc/rfc9239), where 'text/javascript' becomes the standard mimetype for js files.

We decided, therefore, to change the Cache-Control-setting logic to also recognize 'text/javascript'.

Note, in practice, the change in mimetype had no impact on running tensorboards thanks to the logic added in #3128, which always ensures ".js" is mapped to mimetype "application/javascript" but does it at a higher level (ie outside either core plugin or the http utils).
  • Loading branch information
bmd3k committed Jun 8, 2022
1 parent f4d1baf commit 98a8c00
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 2 deletions.
8 changes: 6 additions & 2 deletions tensorboard/plugins/core/core_plugin.py
Expand Up @@ -39,6 +39,11 @@
# If no port is specified, try to bind to this port. See help for --port
# for more details.
DEFAULT_PORT = 6006
# Valid javascript mimetypes that we have seen configured, in practice.
# Historically (~2020-2022) we saw "application/javascript" exclusively but with
# RFC 9239 (https://www.rfc-editor.org/rfc/rfc9239) we saw some systems quickly
# transition to 'text/javascript'.
JS_MIMETYPES = ["text/javascript", "application/javascript"]
JS_CACHE_EXPIRATION_IN_SECS = 86400


Expand Down Expand Up @@ -133,8 +138,7 @@ def _serve_asset(self, path, gzipped_asset_bytes, request):
# Cache JS resources while keep others do not cache.
expires = (
JS_CACHE_EXPIRATION_IN_SECS
if request.args.get("_file_hash")
and mimetype == "application/javascript"
if request.args.get("_file_hash") and mimetype in JS_MIMETYPES
else 0
)

Expand Down
32 changes: 32 additions & 0 deletions tensorboard/plugins/core/core_plugin_test.py
Expand Up @@ -19,6 +19,7 @@
import contextlib
import io
import json
import mimetypes
import os
from unittest import mock
import zipfile
Expand Down Expand Up @@ -150,6 +151,9 @@ def setUp(self):
app = application.TensorBoardWSGI([self.plugin])
self.server = werkzeug_test.Client(app, wrappers.Response)

def tearDown(self):
mimetypes.init()

def _add_run(self, run_name):
run_path = os.path.join(self.logdir, run_name)
with test_util.FileWriter(run_path) as writer:
Expand Down Expand Up @@ -191,6 +195,34 @@ def test_js_no_cache(self):
)

def test_js_cache(self):
"""Tests cache header for js files marked for caching.
The test relies on local system's defaults for the javascript mimetype
which, in practice, should be one of 'text/javascript' or
'application/javascript'. It could be either, depending on the system.
See test_js_cache_with_text_javascript() and
test_js_cache_with_application_javascript() for test cases where
the javascript mimetype have been explicitly configured.
"""
response = self.server.get("/index.js?_file_hash=meow")
self.assertEqual(200, response.status_code)
self.assertEqual(
ONE_DAY_CACHE_CONTROL_VALUE, response.headers.get("Cache-Control")
)

def test_js_cache_with_text_javascript(self):
"""Tests cache header when js mimetype is 'text/javascript'."""
mimetypes.add_type("text/javascript", ".js")
response = self.server.get("/index.js?_file_hash=meow")
self.assertEqual(200, response.status_code)
self.assertEqual(
ONE_DAY_CACHE_CONTROL_VALUE, response.headers.get("Cache-Control")
)

def test_js_cache_with_application_javascript(self):
"""Tests cache header when js mimetype is 'application/javascript'."""
mimetypes.add_type("application/javascript", ".js")
response = self.server.get("/index.js?_file_hash=meow")
self.assertEqual(200, response.status_code)
self.assertEqual(
Expand Down

0 comments on commit 98a8c00

Please sign in to comment.