Revert #2104, provide way to disable disconnects in read_response()

[kristjanvalur]

Pull Request check-list

Please make sure to review and check all of these items:

• Does $ tox pass with this change (including linting)?
• Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Is there an example added to the examples folder (if applicable)?
• Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

PR #2104, a fix to #2103 caused a regression for issue #1128. This has been dicussed in issue #2499.

This PR provides

• A regression test for issue Connections are left in an invalid state on interrupt #1128
• A revert of pr Catch Exception and not BaseException in the Connection #2104, along with comments in the code
• A different way to fix Connection class disconnects on BaseException #2103 with an optional argument to Connection.read_response()

A lot of code uses the Command/Response pattern and if an exception occurs during that, the conneciton is in an
indeterminate state:

• Connection.send_command() has no way to continue sending a command if there is a benign interrupt, e.g. a Timeout. Ensuring that the connection is closed in this case is prudent, because some code does capture BaseExceptions, e.g. gevent style code.
• Code often pairs send_command() and read_response() and even if the error happens in read_response() (which is in principle resumable), application code has no way of knowing and so closing the connection is necessary.

To support PubSub, and other cases where an application may want to properly handle Timeouts and other errors while reading reponses, a new argument is added to read_response(), which disables the automatic disconnect() of the connection when an exception is occurred. It is then the responsibility of the caller to take appropriate action.

[codecov-commenter]

Codecov Report

Patch coverage: 30.76% and project coverage change: -31.81 ⚠️

Comparison is base (e1017fd) 92.29% compared to head (e34f456) 60.49%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2506       +/-   ##
===========================================
- Coverage   92.29%   60.49%   -31.81%     
===========================================
  Files         116      116               
  Lines       29890    29935       +45     
===========================================
- Hits        27588    18110     -9478     
- Misses       2302    11825     +9523     

Impacted Files	Coverage Δ
redis/asyncio/client.py	27.08% <0.00%> (-65.54%)	⬇️
tests/test_asyncio/test_connection.py	39.35% <0.00%> (-58.71%)	⬇️
tests/test_connection.py	68.59% <0.00%> (-29.76%)	⬇️
tests/test_commands.py	70.49% <26.31%> (-19.52%)	⬇️
tests/test_asyncio/test_commands.py	73.64% <33.33%> (-24.72%)	⬇️
redis/asyncio/connection.py	82.54% <37.50%> (-5.08%)	⬇️
redis/connection.py	84.14% <44.44%> (-2.04%)	⬇️
redis/client.py	70.37% <100.00%> (-19.15%)	⬇️

... and 73 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ikonst]

Is this a 2nd phase of the same test?

[kristjanvalur]

Not sure exactly what you're referring to. The 'helper´ gets interrupted, then does another request. The main function then sets out to wait for the 'helper' to finish. The timeout is there to ensure that we finish, even if the helper task hangs.

[ikonst]

nit - even though it's not the test itself, can replace with

Suggested change

	try:
	with pytest.raises(CancelledError):

[ikonst]

Does this also exercise the "command expects int but got back str" scenario?

[kristjanvalur]

No. It excercises the originally reported scenario: A blocking wait is initiated, and even though a new command is sent on the channel, the server is still in blocking wait mode.
It is harder to interrupt a half-read response, although not impossible by using mocking. I can look into that.

[ikonst]

That scenario doesn't require half-read. It's an interrupt between send_command and parse_response:

send_command("EXISTS foo")
<interrupt>
send_command("GET foo")
parse_response() --> expected str, got int (from previous command)

[kristjanvalur]

I'll add a half-read test case.

[ikonst]

Suggested change

	def helper():
	def helper() -> None:

IIUC there's no way to get a retval from a thread, though you can pass a mutable object and use that to communicate.

In particular it might be that we need to marshal errors to avoid them being silently ignored? e.g. https://github.com/bjoluc/pytest-reraise

[kristjanvalur]

The test is relying on the thread actually finishing. There is no way to interrupt a hanging thread in python. If these tests fail, the test suite hangs. I'm open to suggestions on how to avoid that. I suppose that an old-school timeout can be added.

[kristjanvalur]

Also, there is no type hinting in redis-py, not going to start adding that to unit tests :)

[ikonst]

Yeah, I mostly just meant for you to remove the retval. No need to type-annotate, though I do notice that some stuff is type-annotated.

[kristjanvalur]

You're right, there is the odd type hinting, but It is not consistently applied. I guess people add it to help their dev tools with code completion.

[Chronial]

Note that this is not enough to fix everything broken by #2104. At least the other callsites of send_command and read_response are also affected.

[ikonst]

I saw the other send_commands too, but wasn't sure whether to nitpick since they seem to be in distinct flows:

• self.send_command("PING") in SentinelManagedConnection
• connection.send_command("READONLY") in RedisCluster

but overall I'd think those would also do well to be consolidated under a single pattern

[ikonst]

Also, might be good to remove the except Exception: unless there's a specific reason for it.

[kristjanvalur]

Also, might be good to remove the except Exception: unless there's a specific reason for it.

Which one is that?

[ikonst]

Also, might be good to remove the except Exception: unless there's a specific reason for it.

Which one is that?

redis-py/redis/connection.py

Lines 781 to 783 in 67214cc

	except Exception:
	self.disconnect()
	raise

redis-py/redis/connection.py

Lines 819 to 821 in 67214cc

	except Exception:
	self.disconnect()
	raise

[ikonst]

Suggested change

	) as mock_recv:
	):

unless you use mock_recv later

[ikonst]

Note that unless you propagate exceptions from the thread to the test, we'd be unaware of this pytest.raises assertion failing.

[Chronial]

Since you seem really determined to go down this route:

You now disconnect on the outside for any exception. Are you sure this is correct? This will at least disconnect twice sometimes. And are there really no exceptions that should go through without disconnect?

Since you are against rolling back #2104, you should denote the API change in the functions affected, because they now have a rather surprising API. send_command needs a comment explaining that if it raises, the connection is now in an undefined state and the caller needs to disconnect it. Same for read_response and parse_response.

If you succeed with #2510, that comment could change on the async response functions to explaining that if the Exception was a CancellationException, the connection is not undefined, but still holds the previous response. If it is another Exception like KeyboardInterrupt, the connection will still be broken.

[ikonst]

Do we agree that under all scenarios, interrupted send_command:

• leaves the socket in a defined state?
• leaves the redis.Connection in an undefined state (per RESP protocol)?

So expecting socket to handle this (e.g. by disconnecting) would be wrong, since socket will be making assumptions about a higher-level protocol. However, should the responsibility fall unto redis.Connection or unto all its callers?

Arguments in favor of making redis.Connection.send_command responsible:

• Less code repetition
• Lower chance of forgetting to do it at a call site
• Scope: redis.Connection has knowledge of RESP, and is expected to make decisions based on that knowledge

If it is another Exception like KeyboardInterrupt, the connection will still be broken.

Maybe a side-note, but when interruption can happen at any point, as it is with KeyboardInterrupt, I think no approach could protect against an undefined state, so maybe there's no use thinking about it.

[tobymao]

Would it be easier to just roll back #2104?

[ikonst]

@tobymao no, because it was done for a reason

[ikonst]

Still tbd to validate that not only you don't "hang", but also that the next read response matches the command.

[kristjanvalur]

Yes, a separate test which continues an interrupted read seems proper.

[Chronial]

Maybe a side-note, but when interruption can happen at any point, as it is with KeyboardInterrupt, I think no approach could protect against an undefined state, so maybe there's no use thinking about it.

Before #2104 this seemed to work quite well.

[ikonst]

Before #2104 this seemed to work quite well.

You can't guarantee that KeyboardInterrupt doesn't happen twice, once when you recv and another time before you run disconnect(). If something happened, it's by chance, because most of the time you had to Ctrl-C was during lengthy I/O calls.

[Chronial]

In production, where this matters nobody is pressing Ctrl-C. And if they are, they should know not to press it twice if they want sensible shutdown. In the important scenario, like the one you explained before, the exception is raised by a framework. Obviously it should only raise a single one.

[kristjanvalur]

Also, might be good to remove the except Exception: unless there's a specific reason for it.

Right, this needs to be BaseException (naked except clauses are deprectated).

Ok, let's give this PR another whirl.

[kristjanvalur]

You now disconnect on the outside for any exception. Are you sure this is correct? This will at least disconnect twice sometimes. And are there really no exceptions that should go through without disconnect?
Disconnecting twice is safe.
I think we have established that there should be no exceptions going through without a disconnect. That was previously the case with all exceptions while reading a response. Now BaseExceptions don't disconnect, in the Connection but they do cause a disconnect in the scope of a command/response pair.

Since you are against rolling back #2104, you should denote the API change in the functions affected, because they now have a rather surprising API. send_command needs a comment explaining that if it raises, the connection is now in an undefined state and the caller needs to disconnect it. Same for read_response and parse_response.

This is incorrect. It it is only in the case of BaseExceptions that it is kept open, exceptions that are not handled or caught anywhere and typically result in an exit. (In async code, the TimeoutError does result in a disconnect().
However, since there is no way to restart a partially completed send (the API doesn't support it, even if we would keep track of the sent data), I see no harm in closing it. Particularly since it may help the code jive with gevent. I'll make a PR to fix the send_command.

(We could, if we wanted, create a different api which could be restarted when a Timeout has previously occurred.)

If you succeed with #2510, that comment could change on the async response functions to explaining that if the Exception was a CancellationException, the connection is not undefined, but still holds the previous response. If it is another Exception like KeyboardInterrupt, the connection will still be broken.

Fair point. Yes, a timeout on read_response really should mean that the application can retry, and that should be documented, I guess, if the opposite is already undocumented. Otherwise, in all APIs that I am familiar with, a timeout on an operation which provides a "timeout" parameter, means that the operation can be retried.

[Chronial]

You now disconnect on the outside for any exception. Are you sure this is correct? This will at least disconnect twice sometimes. And are there really no exceptions that should go through without disconnect?
Disconnecting twice is safe.

I think we have established that there should be no exceptions going through without a disconnect. That was previously the case with all exceptions while reading a response. Now BaseExceptions don't disconnect, in the Connection but they do cause a
disconnect in the scope of a command/response pair.

Yes: "That was previously the case with all exceptions while reading a response". You extended the scope of the try-catch. Now, for example, any exception raised by a response callback will also disconnect the connection. That was not previously the case.

[kristjanvalur]

Yes: "That was previously the case with all exceptions while reading a response". You extended the scope of the try-catch. Now, for example, any exception raised by a response callback will also disconnect the connection. That was not previously the case.

Okay, I think it is simplest that I restore the original behaviour of connection.parse_response(), since there simply seem to be too many cases that rely on it. Even though none of them was caught by the test suite. I'll modify this PR so that the BaseException behaviour becomes optional.

[ikonst]

Consider: The lack of support is really RESP's (the wire protocol).

[kristjanvalur]

Actually no. You can keep the buffer of data (the message) around in a send buffer. the actual socket.send() message lets you know how much of this has been successfully delivered to underlying layers. So a re-triable API could, for example, accept a None as the data, and then continue sending from where it left off, if it has a buffer in place. Consider this api:

def send_packed_command(message: Optional[str]):
   """ Send the packed command, or, if None, a previous call must not have completed and the function will
        will resume sending that data over the wire.
  """
  ...

[ikonst]

Yes, on the Python side (e.g. in Connection) we could choose to implement a write buffer and "continue where we left off". As I mentioned before, the problem with a write buffer would be the potential of flushing out a command that's stale from an app code's perspective.

What I'd rather see, is being able to, instead of flushing a command that can be stale, to send a short control sequence that would cancel a command that's half-written into the socket, turning it into a no-op. This is the feature that RESP protocol doesn't support.

So:

1. Write buffer: Not an inherent limitation in Python, we simply don't want to do it because it's bad UX.
2. Cancel a half-written command: Inherent limitation of RESP.

Hence the "problem" is RESP, not send_packed_command.

[kristjanvalur]

I see what you are saying, but I'm not even thinking about the "cancel" of the command. That's something I've never seen and in my mind completely unnecessary. For that, you can simply disconnect. I'm primarily thinking about Timeouts, where the transmitting of the command over the wire times out, and you want to maybe do some housekeeping and then continue trying to send the rest of the command.

In the context of this PR, do you suggest that I change this comment somehow? Is it confusing? I always find that code comments help future contributors. A similar comment would have probably caused me to do my original PR differently :)

[ikonst]

Yes, I'm discussing how we phrase the comment :) I think I understand now.

My perspective:
I called send and caught an exception. Next time I call send, it might be a different command, at a different time, on a different thread. I wouldn't want send_packed_command to flush my previous command at an arbitrary time in the future. I would settle for flushing a short "cancel sequence" instead, but RESP doesn't support that, so instead I'd like it to disconnect.

From that perspective, it's a RESP limitation.

Your perspective:
I called send and caught an exception. Now I want to retry immediately, but send_packed_command didn't tell me where it stopped nor allows me to resume half-way.

From that perspective, it's a send_packed_command limitation.

[kristjanvalur]

Yes, I understand. Let me try to formulate the comment better.

[ikonst]

Even if you knew how many bytes were sent (I think you can know), the problem is you can't "take them back".

[kristjanvalur]

It's not about "taking them back", its about "trying again to send the rest". This could (theoretically) be done in case of a timeout, and is the way that many socket.sendall() implementations work. (the underlying socket.send() sends a potential subset of the data and needs to be retried.)

send_packed_api() could be architected in a similar way, but maintain an internal buffer of unsent data so that it can be re-tried.

[ikonst]

But of course it's not reasonable to flush a command after an arbitrary delay (could be milliseconds, could be days). The "workaround" supported by RESP is not reasonable for what Redis is.

[kristjanvalur]

I'm not sure I understand what you mean. All I'm saying is that sometimes your send() can time out, for various reasons, buffers, server responsiveness, or whatever. It's technically possible to engineer things such that you can at least be sure that you've delivered all of your payload down to the OS to deliver over the wire. What happens after that is anyone's guess. Typically, then, you'd listen for the response, and time out in the listening phase if nothing happens.

This is in no way dissimilar to http, and doesn't have anything to do with RESP as such.

Anyway, I thought I'd comment on that, a different API, allowing one to retry a partially completed send() operation, could mean that it would make sense to keep the connection open. But the current API has no semantics that woudl allow a "retry".

[kristjanvalur]

also, see the above discussion :)

[chayim]

@kristjanvalur I see the green passes. Ready for a review?

[kristjanvalur]

Yes, this has been ready for a long time. The non-green are the spurious failures that the test suite has.

[dvora-h]

@chayim Can you review this?

[kristjanvalur]

I'm investigating the test failure.

[kristjanvalur]

This is new behaviour with the 3.11 timeout context manager. It doesn't work if there has been a previous cancel on the task.
python/cpython#102780

[kristjanvalur]

Oh dear. Now it is hanging because it is no longer possible to cancel an operation, due to this code:

        return await asyncio.shield(
            self._try_send_command_parse_response(conn, *args, **options)
        )

in client.py.
The send command and parse is shielded, so cancelling the command won't cancel it. This leaves the __single_conn_lock held.
I wonder why this was added? It breaks things.

[kristjanvalur]

This happened in pr #2666

[ikonst]

This issue is your white whale. I'm sorry :) but thanks for the efforts.

[kristjanvalur]

Oh my. This here PR actually fixes all the other problems, including #2624 and is a much simpler and cleaner fix than the change from #2666 and related commits. I have started a new pr, #2695 , to move this forward and undo those other changes. It would have saved a lot of trouble if I had been pinged about those other issues, I could have pointed to this PR which has been lingering for months.

[kristjanvalur]

Closed this now that #2695 has been merged (a re-creation of this PR)