Providing negative value for RedisCache.set() throws an exception

[gergelypolonkai]

When I provide a negative timeout to RedisCache.set() (except the exact value of -1,) the redis library throws an exception:

  File "/apps/dev/game-app/.venv/lib64/python3.6/site-packages/werkzeug/contrib/cache.py", line 631, in set
    value=dump, time=timeout)
  File "/apps/dev/game-app/.venv/lib64/python3.6/site-packages/redis/client.py", line 2014, in setex
    return self.execute_command('SETEX', name, time, value)
  File "/apps/dev/game-app/.venv/lib64/python3.6/site-packages/redis/client.py", line 573, in execute_command
    return self.parse_response(connection, command_name, **options)
  File "/apps/dev/game-app/.venv/lib64/python3.6/site-packages/redis/client.py", line 585, in parse_response
    response = connection.read_response()
  File "/apps/dev/game-app/.venv/lib64/python3.6/site-packages/redis/connection.py", line 582, in read_response
    raise response
redis.exceptions.ResponseError: invalid expire time in setex

The exact value of -1 (which is also returned by self._normalize_timeout if the timeout value is None) is handled correctly; in this case, no timeout is set.

I think if the provided timeout value is negative, RedisCache (and for that matter, any cache backend) should not cache the value at all (and maybe issue a warning).

I’m using Werkzeug 0.14.1, but the same issue is also present on master.

[davidism]

The problem with this is is broadens the semantics of the argument. If for some reason we give another meaning to -2, that's a breaking change if we previously handled all negative numbers the same. What are you doing that requires passing something besides -1?

[lepture]

BTW, we are going to remove contrib.cache from werkzeug. See #1249

[gergelypolonkai]

This was a result of a bug in my code where I calculated the expiry by diffing now() and a database timestamp; when the timestamp, expected to be in the future, was instead in the past, the result was negative. If it happened to be -1, it would have been cached indefinitely (it may or may not happened; but that’s my fault.)

I still think setting any negative value should be treated specially, not just -1:

if time == -1:
    do_something()
elif time < 0:
    warning('Timeout set to a negative value, won’t cache')
    return

do_the_caching()

This allows you to later use other negative values.

But again, I debate -1 is the correct “special value” here, although I can’t come up with a better one.