Caches Cloudfront Signers

[scuml]

This adds a class-level cache layer in front of cloudfront signers.

Was debugging slowness on a django repo with 50 s3 storage fields, and the expensive load_pem_private_key was called for every field implementing the S3Storage class, even though the id/key was the same for each field. This added a full 3 seconds to server start/restart time.

Caching seems desirable for all users of the library, will provide an instant speed benefit for current users, and will keep django apps snappy and responsive no matter how many filefields they add to their django project.

[jschneier]

Thanks this makes sense. Should this be removed in __getstate__ and __setstate__?

[scuml]

Good idea to check that:

Since it is declared and always treated as a class var and not an instance var, it never shows up in dict, so it would not need to be handled in __getstate__ and __setstate__

>>> class A():
...     class_var = None
...     instance_var = None
...     def __init__(self):
...             self.__class__.class_var = 1
...             self.instance_var = 2
...             print(self.__dict__)
...
>>> A()
{'instance_var': 2}

[jschneier]

Doesn’t line 379 reference as an instance variable?

[scuml]

Correct - remedied.

[jschneier]

Just the lint failure and then good to merge!

[scuml]

Ah - yes, line length is why I removed class on that line during development. Linted and done.

[jschneier]

Huh well that's a lint error I haven't seen before.

[scuml]

Python typing was a mistake.... i think this will fix it.

[scuml]

Looks like that one throws a number of false positives. https://github.com/astral-sh/ruff/issues/5243

[jschneier]

Python typing was a mistake....

Amen.

[jschneier]

This is what it wants (verified locally)

diff --git a/storages/backends/s3.py b/storages/backends/s3.py
index 98b6e94..76dec89 100644
--- a/storages/backends/s3.py
+++ b/storages/backends/s3.py
@@ -4,6 +4,7 @@ import os
 import posixpath
 import tempfile
 import threading
+import typing
 import warnings
 from datetime import datetime
 from datetime import timedelta
@@ -30,8 +31,6 @@ from storages.utils import safe_join
 from storages.utils import setting
 from storages.utils import to_bytes

-from typing import Dict
-
 try:
     import boto3.session
     import botocore
@@ -313,7 +312,7 @@ class S3Storage(CompressStorageMixin, BaseStorage):
     # settings/args are ignored.
     config = None

-    _signers: Dict[str, str] = {}
+    _signers: typing.ClassVar = {}

     def __init__(self, **settings):
         omitted = object()

[jschneier]

Alternatively we could update it to ignore the error via a comment. Either is fine for me as I don't find this annotation very helpful.

[scuml]

Thanks for merging. My leaning would be to ignore as the classvar type a) conveys nothing of value to the user and in a way obfuscates the fact it's a simple dict, and b) would avoid confusion later with future contributors wondering why this single type exists, and if they are now obligated to litter the rest of a very readable codebase with types.

And now I'm checking the commits and seeing that's exactly what you did.
Well played.