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

Add ETag header for static responses #2306

Merged
merged 3 commits into from Mar 17, 2024
Merged

Conversation

redraw
Copy link
Contributor

@redraw redraw commented Mar 15, 2024

Related to #1645

This PR adds ETag headers for static responses. Adding cache-control headers was previously discussed but nothing done yet, so I've thought adding ETag headers would drastically reduce bandwidth meanwhile. Specially, when quickly deploying to Vercel or Fly, and you don't own a domain, required to add a CDN on front.


📚 Documentation preview 📚: https://datasette--2306.org.readthedocs.build/en/2306/

@redraw redraw marked this pull request as draft March 15, 2024 13:55
@redraw redraw changed the title Add ETag for static responses Add ETag header for static responses Mar 15, 2024
@redraw
Copy link
Contributor Author

redraw commented Mar 15, 2024

I've branched from latest main, but black found files to be reformatted.

Oh no! 💥 💔 💥
65 files would be reformatted, 32 files would be left unchanged.

EDIT: found out it was my ~/.config/black which set a different line-length

@redraw redraw force-pushed the etag-for-static-files branch 8 times, most recently from 2fba5f9 to 89b5574 Compare March 16, 2024 16:41
@redraw redraw marked this pull request as ready for review March 16, 2024 16:42
@redraw
Copy link
Contributor Author

redraw commented Mar 16, 2024

Done, and tests passes. However, there's still a weird runtime error. Checking now.

Traceback (most recent call last):
  File "/home/agustin/tmp/datasette/datasette/app.py", line 1668, in route_path
    response = await view(request, send)
  File "/home/agustin/tmp/datasette/datasette/utils/asgi.py", line 337, in inner_static
    await asgi_send_file(
  File "/home/agustin/tmp/datasette/datasette/utils/asgi.py", line 305, in asgi_send_file
    await send(
  File "/home/agustin/tmp/datasette/env/lib/python3.10/site-packages/asgi_csrf.py", line 104, in wrapped_send
    await send(event)
  File "/home/agustin/tmp/datasette/env/lib/python3.10/site-packages/uvicorn/protocols/http/h11_impl.py", line 514, in send
    output = self.conn.send(event=h11.EndOfMessage())
  File "/home/agustin/tmp/datasette/env/lib/python3.10/site-packages/h11/_connection.py", line 512, in send
    data_list = self.send_with_data_passthrough(event)
  File "/home/agustin/tmp/datasette/env/lib/python3.10/site-packages/h11/_connection.py", line 545, in send_with_data_passthrough
    writer(event, data_list.append)
  File "/home/agustin/tmp/datasette/env/lib/python3.10/site-packages/h11/_writers.py", line 67, in __call__
    self.send_eom(event.headers, write)
  File "/home/agustin/tmp/datasette/env/lib/python3.10/site-packages/h11/_writers.py", line 96, in send_eom
    raise LocalProtocolError("Too little data for declared Content-Length")
h11._util.LocalProtocolError: Too little data for declared Content-Length

@redraw
Copy link
Contributor Author

redraw commented Mar 16, 2024

Might be related to how I'm forwarding headers.

@redraw
Copy link
Contributor Author

redraw commented Mar 16, 2024

Done! fixed.

@redraw
Copy link
Contributor Author

redraw commented Mar 16, 2024

image

tests/test_utils.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 17, 2024

Codecov Report

Attention: Patch coverage is 96.29630% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.45%. Comparing base (8b6f155) to head (b440c2c).
Report is 4 commits behind head on main.

Files Patch % Lines
datasette/utils/asgi.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2306   +/-   ##
=======================================
  Coverage   92.44%   92.45%           
=======================================
  Files          42       42           
  Lines        6420     6441   +21     
=======================================
+ Hits         5935     5955   +20     
- Misses        485      486    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@simonw
Copy link
Owner

simonw commented Mar 17, 2024

This is clearly a good idea. Adding it directly to asgi_static() looks like the right place, too. I was initial concerned that this might interfere with how /database.db downloads work - the most performance-sensitive part of the code for this because the file might be 1GB+ and I wouldn't want to have to calculate MD5 against that every time - but those are not served by asgi_static() and in fact it turns out they have ETag support already:

if db.hash:
etag = '"{}"'.format(db.hash)
headers["Etag"] = etag
# Has user seen this already?
if_none_match = request.headers.get("if-none-match")
if if_none_match and if_none_match == etag:
return Response("", status=304)
headers["Transfer-Encoding"] = "chunked"
return AsgiFileDownload(
filepath,
filename=os.path.basename(filepath),
content_type="application/octet-stream",
headers=headers,
)

@redraw
Copy link
Contributor Author

redraw commented Mar 17, 2024

Yes. I tried adding it inside asgi_send_file() first, then a related test failed and found out that same part where you added ETag headers for downloading the whole db. Then, I realized it was better to move it outside it and asgi_static() was safe.

@simonw
Copy link
Owner

simonw commented Mar 17, 2024

I deployed this branch to my own Datasette Cloud instance - here's the before:

CleanShot 2024-03-17 at 12 02 42@2x

And after:

CleanShot 2024-03-17 at 12 14 20@2x

Huge improvement!

@simonw simonw merged commit 67e66f3 into simonw:main Mar 17, 2024
11 checks passed
@redraw redraw deleted the etag-for-static-files branch March 17, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants