BaseException at I/O corrupts Connection

[ikonst]

tl;dr repeat of #360

My codebase uses gevent (w/monkey-patching) for concurrent workers, and schedules a gevent.Timeout to force a deadline on the workers. Since a timeout causes an exception to be raised from an arbitrary "yield point", there's risk of corrupting shared state if code is not written to be exception-safe.

The code talks to redis over a client that's shared between the workers. Socket I/O is a "yield point". Sending a command successfully, but then failing to read the entire response off the socket, gets Connection into an undefined state: a subsequent command would try to parse a response intended for a previous command, and in many cases would fail. Here's what a typical error would look like:

  File "site-packages/redis/client.py", line 3401, in parse_response
    result = Redis.parse_response(
  File "site-packages/redis/client.py", line 774, in parse_response
    return self.response_callbacks[command_name](response, **options)
  File "site-packages/redis/client.py", line 528, in <lambda>
    'SET': lambda r: r and nativestr(r) == 'OK',
  File "site-packages/redis/_compat.py", line 135, in nativestr
    return x if isinstance(x, str) else x.decode('utf-8', 'replace')
AttributeError: 'int' object has no attribute 'decode'

As you can see, we're trying to parse an integer response to a previous command (e.g. ":12345\r\n") as a string response to a SET command.

The except Exception block in redis.connection.Connection.read_response is intended to handle all sorts of parsing errors by disconnecting the connection:

redis-py/redis/connection.py

Lines 819 to 821 in 6219574

	except Exception:
	self.disconnect()
	raise

but perhaps it could be changed to except: since e.g. gevent.Timeout is intentionally a BaseException to suggest that it should not be suppressed (and we are indeed not suppressing it).

[ikonst]

Digging through history:

• this was first reported in Connections can end up in inconsistent state due to except Exception: instead of bare except: #360 in 2013
• then regressed in 3.0.0
• then again reported in Connections are left in an invalid state on interrupt #1128 and fixed in Do not leave connections in invalid state #1129 (cc @Chronial), released in 3.2.0
  • but alas without a regression test, which didn't add any controls for it being reverted
• then Connection class disconnects on BaseException #2103 got reported
• then got reverted in Catch Exception and not BaseException in the Connection #2104 (cc @kristjanvalur), released in 4.4.0rc2
  • Apparent motivation: pubsub without disconnects (though PubSub.on_connect seems to re-establish subscriptions, but there's probably a brief loss)

[ikonst]

P.S. I can think of a few alternatives to self.disconnect() where the onus on recovery is on the next caller rather than the current one (thus we don't have to do risky things in an exception handler).

1. After send_command, increase a self._expected_responses counter. In parse_response, decrease it. Before send_command, ensure it's down to zero by running _ = self.parse_response() as many times as needed.
2. Without maintaining a counter, try and flush the socket by reading all that's on it before send_command. I suspect it's race-y since nothing guarantees that the previous command's response won't arrive right after that flush.

I suspect we chose to self.disconnect() because overhead of connecting to redis is low, and an exception might suggest some other form of persistent corruption?

Update: After considering I've realized this is fundamentally unsafe to continue using the socket, since we don't know how much data was sent by sendall nor how much was read off the socket by recv.

[ikonst]

I need to better understand motivation of #2104 since it's directly at odds with this issue. It appears to focus on PubSub. Since PubSub makes the connection receive ad-hoc responses (in addition to responses to commands), a disconnection probably causes a brief data loss.

Since PubSub responses are designed to be unambiguous — response is a list whose first element indicates message type, e.g. "subscribe" for a response to user command, or "message" for a server-pushed message — a possible compromise is that PubSub triggers a special mode in Connection which disables disconnects on BaseException.

[kristjanvalur]

Generally you want to avoid logic on BaseExceptions. They are either fatal (thread exit) or intended as signalling exceptions, such as timeout. Code running in BaseException handlers can cause trouble when it, itself, raises exceptions.

Typically what happens is that the BaseException is unhandled and then dealt with at a higher level, such as application level, by discarding the connection. It is most unusual to do this at such a low level.

I think that the motivation behind the change in the Synchronous api was to better reflect that coding practice. Leave BaseExceptions in place, because they usually should be left alone. Handlers then do the right thing at the place where the error is caught.

Normally, I'd suggest you catch the gevent.Timout exception higher up. You are catching it somewhere, otherwise you'd not be able to timeout. You can then decide what to do. Drop the connection? Retry reading the response? It is an application level decision. The low level api should not be making that choice for you.
However, timeout semantics in the Synchronous api are all over the place and it may well be that the behaviour you describe is best.

[Chronial]

Generally you want to avoid logic on BaseExceptions.

This is simply not true and does not reflect a sensible coding practice. A BaseException means that the stackframe was left at an unexpected point and should be handled exactly as such. If you have code for which this matters, you need to react to the BaseException.

Typically what happens is that the BaseException is unhandled and then dealt with at a higher level, such as application level, by discarding the connection. It is most unusual to do this at such a low level.

You seem to be missing the point of the exception handler in the send_packed_command function: Transmission of data failed in the middle of a command, the redis protocol (which the Connection object encapsulates) is now in an undefined state. The connection is completely useless, because you are de-synchronized from the server. There is nothing you can do to remedy this – the connection needs to be closed.

If you had actually taken the time to look at the history of the code you where modifying in #2104 – as @ikonst did – you would already know all of this.

Normally, I'd suggest you catch the gevent.Timout exception higher up. [...] You can then decide what to do. Drop the connection?

You can not "deal with [this] at a higher level", because you now lack the necessary information to understand what happened. There is no way to know from the outside that the connection is garbage. To fix this, instead of closing the connection, the Exception handler could set a is_broken_needs_closing flag on the Connection object, but I fail to see what the point of that would be?

In #2103 you wrote

The Timout should leave the connection in the state that it was, [...]

which is wishing for the impossible. You interrupted an operation while it was executing – unless you have a transactional system like in a database, you can't go back to the "state that it was".

I understand your use-case in #2104 – since the connection is mostly idle during pubsub, you don't think you'll interrupt an ongoing transmission, but assume that the timeout will happen while the connection is idle. But:

• This is only ever possible in this one specific case of pubsub: Usually redis connections are not "idle" during the execution of command. While fixing your specific issue, you just strictly broke the code for everybody else
• You also broke your own use case, because you might still interrupt the code while data is being transmitted, which would again leave the connection in a broken state. Your are simply hoping for that not to happen

Your whole approach is broken: You can't just use an external package to force timeout exceptions into network protocols and assume that connections will remain in a usable state. redis-py needs to handle the timeout itself. I just had a look the API here, and you will find that it does exactly that: The get_message method of PubSub takes a timeout parameter. That is what you should have been using all along instead of breaking the low-level implementation of the Connection class.

[ikonst]

Code running in BaseException handlers can cause trouble when it, itself, raises exceptions.

I agree that exception handling is sensitive for BaseException. However, finally clauses effectively do that, so this is done more often that it may seem.

Also, not sure if we can rely on docs for best practices, but asyncio.CancelledError says "In almost all situations the exception must be re-raised." (not "never catch this").

Normally, I'd suggest you catch the gevent.Timout exception higher up. You are catching it somewhere, otherwise you'd not be able to timeout. You can then decide what to do. Drop the connection? Retry reading the response? It is an application level decision. The low level api should not be making that choice for you.

This is unfortunately not possible, since the application code has a RedisClient object but has no access to the underlying connection. In the case of a connection pool, it doesn't even know which connection is the "undefined state" one. (The exception is an arbitrary one, so it can't carry additional context to the app code.)

[kristjanvalur]

If the application doesn't have the connection, someone else has. This code exists on the Connection object. The connection itself should not decide that it has failed if it encounters, e.g. a timeout. It is whoever is using the connection, in whatever manner it is doing that, who should. If you have a Redis which is picking arbitrary connections out of a pool, and returning them, it is this Redis which knows how it is using the connections and that a Timeout is actually fatal. How does the rest of your callstack look? There is almost certainly a better place, higher up, where it can better be decided that an exception invalidates this connection.

[Chronial]

The connection itself should not decide that it has failed if it encounters, e.g. a timeout

Did you actually read what I wrote? The connection is the object that gets irreparably broken, which only the object itself can know. Why should the object not "decide" that?

This is not about timeouts. This is about low-level networking operations failing. In your case because you forced a timeout exception into them.

[kristjanvalur]

A forced timeout is not a failing network exception. It is resumable. Even under gevent. The connection is still in a perfectly valid state. The read_response() method can be re-invoked on the Connection after a timeout. The connection itself is not "irrepairably broken".

Recall, a Connection can be used for three different things, IIRC:

1. It can be used in a connection pool, where one is picked for executing each command, and retrieving a response.
2. It can be used directly, by an application, without a pool.
3. It can be used by a PubSub instance. Again, a single connection, managed throughout.

In only the first case must it be closed and flushed if a full command-response is not performed. This is not something which the Connection itself should be deciding, it is whichever of the layers above, which is using the connection (i.e. ConnectionPool), should be doing.

Having said that, there are actually two different timeouts in operation in Redis. One is the "socket timeout" which is applied to each operation, and ususally considered in the same way as other socket errors, i.e., "something happened on the network and we should just consider it as fatal", and then there is the user "timeout" which is a single operation timeout, applied to each operation from the api, but for the user to decide how to respond to. This does complicate things. In the Async code I have cleaned up the handling of these two so that they don't interfere with each other and are processed in a cleaner way.

[Chronial]

A forced timeout is not a failing network exception. It is resumable. Even under gevent. The connection is still in a perfectly valid state. The read_response() method can be re-invoked on the Connection after a timeout. The connection itself is not "irrepairably broken".

This is not correct. We are also not really talking about "timeouts" – the code you changed had nothing to do with timeouts. What you are claiming (with your words and your code change) is: "Any BaseException that is not an Exception thrown during socket operations leaves the socket and redis protocol in a valid state". Once again, that is not true.

Recall, a Connection can be used for three different things, IIRC: [...] In only the [connection pool] case must it be closed and flushed [...]

This is not correct. I do not see any connection pools in the code example I provided (3 years ago) in #1128, do you?

Having said that, there are actually two different timeouts in operation in Redis.

Just to be very clear here: You forcing exceptions into the middle of IO operations is not a timeout. It is simply an unexpected exception.

But this discussion doesn't really seem to go anywhere. Can you maybe instead just answer two simple questions:

1. Why can you not just use the already existing get_message API for your timed wait requirements?
2. How do you suggest code like my example in Connections are left in an invalid state on interrupt #1128 is supposed to be written after your change? Of course in reality, the brpop is deep with in a call tree that does many redis operations and many other things.

[kristjanvalur]

There is a very aggressive tone in this thread which makes me disinclined to respond. However, please allow me to answer your comments in turn in a calm voice.

1. Base exceptions have nothing to do with the state of the socket. They are usually signalling exceptions. No base exception you have mentioned thus far will leave the connection in an unknown state. One should normally only handle exceptions that are known, and leave others. Socket errors, for example, are handled.
2. I'm sorry if I haven't read up on defects provided three years ago. But let's see... Yes, your Redis instance is using a ConnectionPool.
3. Generally, exceptions should be handled at the layer where they are meaningful. the Connection object only has knowledge about certain exceptions which it knows that make the connection defunct, such as SocketError, et al.
4. The changes in the Synchronous API part were made as a clean-up effort, to make the synchronous logic better reflect the Asynchronous and to adhere to good architecture.
5. My suggestion here is that the Redis object be fixed to not resubmit connections in an incomplete state to the pool.

This regression should, of course, have been caught at the time of my PR, and probably would have been if unittest coverage had been adequate. There aren't many regression tests present. The changes were made in good faith.

If you would kindly provide me with a Traceback which demonstrates your error, I'll be happy to create a PR, with regression tests, as appropriate. The callstack would help guide me to make the correct design. Please put a breakpoint at the place where previously there would have been a "disconnect()" call.

We should, of course, ensure that the software works as intended, but it is important to do so in an architecturally sound way. The logic of a Redis instance should not be pushed down into the bowels of a Connection object.

Cheers!

[ikonst]

@kristjanvalur, agreed. I also noticed the aggressive tone (e.g. "Your whole approach is broken", "this discussion doesn't really seem to go anywhere"). It understandably comes out of frustration over this issue being reintroduced twice (in 3.0 and in 4.2), but let's assume best intentions, good faith, and keep looking forward. Overall not a lot of people actually contribute to open source.

If you would kindly provide me with a Traceback which demonstrates your error, I'll be happy to create a PR, with regression tests, as appropriate. The callstack would help guide me to make the correct design. Please put a breakpoint at the place where previously there would have been a "disconnect()" call.

I'm adding a regression test in #2505. Let's get it merged before we address the problem.

You forcing exceptions into the middle of IO operations is not a timeout.

FWIW in event-loop async frameworks like asyncio or gevent, I'm not sure "IO timeouts" and "generic timeouts" are materially different.

Why can you not just use the already existing get_message API for your timed wait requirements?

If what I say above ☝️ is right and timeouts are timeouts, then fundamentally some operations are safe to interrupt and some aren't. get_message might be safe because it's "passive" (pull) rather than "active" (request-response) and whatever message you're interrupted while reading, you'd read later.

In effect, this reenforces @kristjanvalur's point that the knowledge on whether something is interruptible belongs to a higher layer.

[Chronial]

@kristjanvalur

I'm sorry if I haven't read up on defects provided three years ago.

When you changed the exception handler, did it not seem strange to you that someone would handle a BaseException there? You are not supposed to "read up on defects provided three years ago". But I think it's sensible that if you change a piece of code that you don't fully understand, to have a look at its history first to at least try to understand why it's there.

But @ikonst is of course correct – me airing my grievances over this by participating in this discussion in an overly aggressive tone is not a productive approach. I apologize for that.

In case @ikonst's regression test in 2500 doesn't clarify the situation:

No base exception you have mentioned thus far will leave the connection in an unknown state.

The KeyboardInterrupt raised in the example in #1128 leaves the connection of r in an unknown state. This is not caused by the connection pool – passing single_connection_client=True to the Redis constructor doesn't change the outcome.

If you would kindly provide me with a Traceback which demonstrates your error, I'll be happy to create a PR, with regression tests, as appropriate. The callstack would help guide me to make the correct design. Please put a breakpoint at the place where previously there would have been a "disconnect()" call.

That's the traceback:

Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File ".../lib/python3.9/site-packages/redis/commands/core.py", line 2512, in brpop
    return self.execute_command("BRPOP", *keys)
  File ".../lib/python3.9/site-packages/redis/client.py", line 1258, in execute_command
    return conn.retry.call_with_retry(
  File ".../lib/python3.9/site-packages/redis/retry.py", line 46, in call_with_retry
    return do()
  File ".../lib/python3.9/site-packages/redis/client.py", line 1259, in <lambda>
    lambda: self._send_command_parse_response(
  File ".../lib/python3.9/site-packages/redis/client.py", line 1235, in _send_command_parse_response
    return self.parse_response(conn, command_name, **options)
  File ".../lib/python3.9/site-packages/redis/client.py", line 1275, in parse_response
    response = connection.read_response()
  File ".../lib/python3.9/site-packages/redis/connection.py", line 812, in read_response
    response = self._parser.read_response(disable_decoding=disable_decoding)
  File ".../lib/python3.9/site-packages/redis/connection.py", line 318, in read_response
    raw = self._buffer.readline()
  File ".../lib/python3.9/site-packages/redis/connection.py", line 249, in readline
    self._read_from_socket()
  File ".../lib/python3.9/site-packages/redis/connection.py", line 192, in _read_from_socket
    data = self._sock.recv(socket_read_size)
KeyboardInterrupt

The disconnect call used to here.

A good argument could by made that it is actually not enough, as an exception injected into _send_command_parse_response before the call to read_response will also break the connection and was never handled. I guess that was never reported because the probability of that happening is way lower than it happening during the IO calls.