Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33681 closed Bug (fixed)

Cache OPTIONS are not passed to the Redis client.

Reported by: Ben Picolo Owned by: Mariusz Felisiak
Component: Core (Cache system) Version: 4.0
Severity: Release blocker Keywords:
Cc: Nick Pope Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Ben Picolo)

Hello! First Django interaction, so please let me know how better I can give info here.

I discovered unintentionally that there's no default socket_timeout set for Redis cache connections. This is an issue on it's own (unsafe default that's a particularly hard issue to track down), but in trying to set those timeouts, I hit some tough edges of documentation/usage.

The cache documentation says that the OPTIONS object is passed to the third-party connection class for connections backed by third-party libraries for connections backed by third-party libraries. This is reaffirmed for the redis cache specifically here.

Right now, though, these OPTIONS are passed to the first-party RedisCacheClient object, so you can't pass in options expected by the redis connection pool link. In that sense, RedisCacheClient is being treated as a "first-party" cache according to these docs, but it defers directly to the redis library.

One could do a variety of subclassing to make this work, but there's quite a few layers of indirection here, so it's hard to identify exactly what one should subclass in order to get arguments to the connection class appropriately.

(For now, my workaround is take advantage of the from_url behavior where the connection class pulls arguments out of the connection query string, but this isn't straightforward and feels brittle to django implementation details).

There are a couple of options here – revising documentation or changing behavior (or both).

In my ideal case, it would be great for it to be easy to pass down explicit socket and other timeouts. To me, that suggests updating behavior is ideal, because the documentation would potentially lead folk down a path of tough to navigate behavior. Passing those options in tact, or updating the RedisCacheClient to have a wider range of available options would both work out.

Change History (12)

comment:1 by Ben Picolo, 3 years ago

Description: modified (diff)

comment:2 by Ben Picolo, 3 years ago

Description: modified (diff)

comment:3 by Ben Picolo, 3 years ago

Description: modified (diff)

comment:4 by Ben Picolo, 3 years ago

Description: modified (diff)

comment:5 by Ben Picolo, 3 years ago

Description: modified (diff)

comment:6 by Mariusz Felisiak, 3 years ago

Cc: Nick Pope added
Severity: NormalRelease blocker
Summary: Redis client OPTIONS don't work as documented, which makes setting Redis timeouts difficultCache OPTIONS are not passed to the Redis client.
Triage Stage: UnreviewedAccepted
Type: Cleanup/optimizationBug

Thanks for the report. Agreed, we should pass all options to the Redis client. Would you like to prepare a patch? It's probably enough to pass the OPTIONS to the underlying client, e.g.

  • django/core/cache/backends/redis.py

    diff --git a/django/core/cache/backends/redis.py b/django/core/cache/backends/redis.py
    index e0d30784ff..51701b77b3 100644
    a b class RedisCacheClient:  
    3535        db=None,
    3636        pool_class=None,
    3737        parser_class=None,
     38        **options,
    3839    ):
    3940        import redis
    4041
    class RedisCacheClient:  
    5859            parser_class = import_string(parser_class)
    5960        parser_class = parser_class or self._lib.connection.DefaultParser
    6061
     62        self._options = options
    6163        self._pool_options = {"parser_class": parser_class, "db": db}
    6264
    6365    def _get_connection_pool_index(self, write):
    class RedisCacheClient:  
    8183        # cache client can be implemented which might require the key to select
    8284        # the server, e.g. sharding.
    8385        pool = self._get_connection_pool(write)
    84         return self._client(connection_pool=pool)
     86        return self._client(connection_pool=pool, **self._options)
    8587
    8688    def add(self, key, value, timeout):
    8789        client = self.get_client(key, write=True)

Marking as a release blocker as this is as a bug in the new feature.

comment:7 by Ben Picolo, 3 years ago

I could probably give that a go, will see if I can take it on this weekend

in reply to:  7 comment:8 by Mariusz Felisiak, 3 years ago

Replying to Ben Picolo:

I could probably give that a go, will see if I can take it on this weekend

Sounds good!

comment:9 by Mariusz Felisiak, 3 years ago

Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:10 by Mariusz Felisiak, 3 years ago

Has patch: set

comment:11 by GitHub <noreply@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In d27e6b23:

Fixed #33681 -- Made Redis client pass CACHESOPTIONS to a connection pool.

Thanks Ben Picolo for the report.

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 5c6ebe19:

[4.0.x] Fixed #33681 -- Made Redis client pass CACHESOPTIONS to a connection pool.

Thanks Ben Picolo for the report.
Backport of d27e6b233f83c3429f21ff3c250a28ff302637ef from main

Note: See TracTickets for help on using tickets.
Back to Top