Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33061 closed Bug (fixed)

ValueError not raised by some cache backends for incr() / decr() with negative delta when a key is missing

Reported by: Chris Jerdonek Owned by: Sondre Lillebø Gundersen
Component: Core (Cache system) Version: 3.2
Severity: Normal Keywords:
Cc: Sondre Lillebø Gundersen Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Chris Jerdonek)

I was looking at the code for some of the cache backends, and I noticed that in a couple places, it looks like ValueError isn't being raised when it should be. This is in the incr() and decr() methods of BaseMemcachedCache when delta is less than zero. The commit where this code was added is here: https://github.com/django/django/commit/79dd751b0b9e428bf386d7304f442fa9815fdbba

This is just by looking at the code, so I could be wrong. But I wanted to flag it.

Change History (16)

comment:1 by Chris Jerdonek, 3 years ago

Description: modified (diff)

comment:2 by Chris Jerdonek, 3 years ago

Summary: ValueError not raised for incr / decr with negative delta for some backends?ValueError not raised by some cache backends for incr() / decr() with negative delta?

comment:3 by Mariusz Felisiak, 3 years ago

Resolution: needsinfo
Status: newclosed

... it looks like ValueError isn't being raised when it should be.

Why do you think that we should raise an error in this case? I found nothing in our docs or code to suggest that negative delta is forbidden 🤔 It works on all cache backends.

comment:4 by Chris Jerdonek, 3 years ago

Why do you think that we should raise an error in this case?

It's not that an error should be raised from nothing. It's that ValueError should be raised instead of LibraryValueNotFoundException (or returning None) when a key is missing. If you look at the links to the code I provided, the delta < 0 code path isn't inside the try-except block a line later where that conversion takes place for the delta >= 0 case. This would affect only some backends that derive from BaseMemcachedCache. I'm including the incr() method in full below to make it easier to see. The comment below has more details:

def incr(self, key, delta=1, version=None):
    key = self.make_key(key, version=version)
    # memcached doesn't support a negative delta
    if delta < 0:
        return self._cache.decr(key, -delta)
    try:
        val = self._cache.incr(key, delta)

    # python-memcache responds to incr on non-existent keys by
    # raising a ValueError, pylibmc by raising a pylibmc.NotFound
    # and Cmemcache returns None. In all cases,
    # we should raise a ValueError though.
    except self.LibraryValueNotFoundException:
        val = None
    if val is None:
        raise ValueError("Key '%s' not found" % key)
    return val

Also, here is where it's documented in the Django docs that incr() / decr() should raise ValueError when a key is missing:

A ValueError will be raised if you attempt to increment or decrement a nonexistent cache key.:

Last edited 3 years ago by Chris Jerdonek (previous) (diff)

comment:5 by Chris Jerdonek, 3 years ago

Summary: ValueError not raised by some cache backends for incr() / decr() with negative delta?ValueError not raised by some cache backends for incr() / decr() with negative delta when a key is missing

comment:6 by Mariusz Felisiak, 3 years ago

Resolution: needsinfo
Status: closednew
Triage Stage: UnreviewedAccepted

Thanks for the clarification. I misunderstood your original intentions, sorry 🤦.

Also, it looks like we can completely remove BaseMemcachedCache.decr() (as a separate cleanup) after fixing this.

in reply to:  4 ; comment:7 by Sondre Lillebø Gundersen, 3 years ago

Replying to Chris Jerdonek:

Why do you think that we should raise an error in this case?

It's not that an error should be raised from nothing. It's that ValueError should be raised instead of LibraryValueNotFoundException (or returning None) when a key is missing. If you look at the links to the code I provided, the delta < 0 code path isn't inside the try-except block a line later where that conversion takes place for the delta >= 0 case.

Hi!

I was looking at trying to implement this ticket, but I'm confused by one thing (perhaps I'm missing something):

*Won't* the delta < 0 code path for incr raise a ValueError even though it's not in the incr try/except block? Since it calls decr and that method has its own mirrored logic, it seems like you would end up with a ValueError either way, wouldn't you?

    def incr(self, key, delta=1, version=None):
        key = self.make_key(key, version=version)
        # memcached doesn't support a negative delta
        if delta < 0:
            return self._cache.decr(key, -delta)
        try:
            val = self._cache.incr(key, delta)

        # python-memcache responds to incr on non-existent keys by
        # raising a ValueError, pylibmc by raising a pylibmc.NotFound
        # and Cmemcache returns None. In all cases,
        # we should raise a ValueError though.
        except self.LibraryValueNotFoundException:
            val = None
        if val is None:
            raise ValueError("Key '%s' not found" % key)
        return val

    def decr(self, key, delta=1, version=None):
        key = self.make_key(key, version=version)
        # memcached doesn't support a negative delta
        if delta < 0:
            return self._cache.incr(key, -delta)
        try:
            val = self._cache.decr(key, delta)

        # python-memcache responds to incr on non-existent keys by
        # raising a ValueError, pylibmc by raising a pylibmc.NotFound
        # and Cmemcache returns None. In all cases,
        # we should raise a ValueError though.
        except self.LibraryValueNotFoundException:
            val = None
        if val is None:
            raise ValueError("Key '%s' not found" % key)
        return val

The missing key would not be raised from incr but from decr, right? Perhaps I'm missing the point 🙂

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

Replying to Sondre Lillebø Gundersen:

The missing key would not be raised from incr but from decr, right? Perhaps I'm missing the point 🙂

You're missing that it calls self._cache.decr() not self.decr().

in reply to:  8 ; comment:9 by Vijay , 3 years ago

Replying to Mariusz Felisiak:

Replying to Sondre Lillebø Gundersen:

The missing key would not be raised from incr but from decr, right? Perhaps I'm missing the point 🙂

You're missing that it calls self._cache.decr() not self.decr().

Can you please tell me what does self._cache.decr() return for a missing key ?

Last edited 3 years ago by Vijay (previous) (diff)

in reply to:  9 comment:10 by Mariusz Felisiak, 3 years ago

Replying to Vijay :

Can you please tell me what does self._cache.decr() return for a missing key ?

The same as self._cache.incr(), it depends on the library and it's described in the code. This doesn't really matter for the ticket.

comment:11 by Sondre Lillebø Gundersen, 3 years ago

Owner: changed from nobody to Sondre Lillebø Gundersen
Status: newassigned

comment:12 by Sondre Lillebø Gundersen, 3 years ago

Cc: Sondre Lillebø Gundersen added
Has patch: set

Opened at PR for this: https://github.com/django/django/pull/14810 :)

As far as I can tell, it should now work as I expected it to originally - and I think that fixes any uncertainty around the handling of a missing key, since negative deltas are now treated the same as positive deltas, just with the extra step.

comment:13 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

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

In 96ab3a1:

Refs #33061 -- Added DummyCache.incr()/decr() tests for nonexistent keys with negative deltas.

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

Resolution: fixed
Status: assignedclosed

In 2c912c34:

Fixed #33061 -- Fixed handling nonexistent keys with negative deltas in incr()/decr() in memcached backends.

Thanks Chris Jerdonek for the review.

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

In 93e06f29:

Refs #33061 -- Removed unnecessary BaseMemcachedCache.decr().

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