Optionally disable disconnects in read_response

[kristjanvalur]

This PR is a re-submit of #2506 to start with a clean slate.
That PR also fixed all the recent issues including #2624 , which prompted recent changes to async code that are not necessary.

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

Please see description of #2506 for the original reason for that pull request.

In addition, the relevant changes also solve recently observed issues with CancelledError causing connections to
remain in indeterminate state.

1. We fix the original regression, allowing overrides for when we do want to not terminate connection.
2. We undo changes from pr Minor fixes for #2666 and enhanced async test #2673 and related changes, bringing the client.py and cluster.py files to their original form.
3. We fix the test suite in test_cwe_404.py to be more reliable and less reliant on actual timing.
4. We add a host_port_remap feature to RedisCluster (see https://redis-py-cluster.readthedocs.io/en/stable/client.html)
5. We fixe the tests for cluster mode which were previously not doing anything because RedisCluster ignored the ports for
   the DelayProxy. the host_port_remap feature is used to ensure that the connections go through the DelayProxy objects.

[codecov-commenter]

Codecov Report

Patch coverage: 71.17% and project coverage change: -13.90 ⚠️

Comparison is base (e52fd67) 92.30% compared to head (447c732) 78.41%.

📣 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    #2695       +/-   ##
===========================================
- Coverage   92.30%   78.41%   -13.90%     
===========================================
  Files         116      116               
  Lines       30093    30169       +76     
===========================================
- Hits        27778    23656     -4122     
- Misses       2315     6513     +4198     

Impacted Files	Coverage Δ
redis/asyncio/cluster.py	16.93% <0.00%> (-75.30%)	⬇️
redis/asyncio/connection.py	85.59% <50.00%> (-2.17%)	⬇️
redis/connection.py	85.68% <55.55%> (-0.53%)	⬇️
tests/test_asyncio/test_cwe_404.py	71.25% <67.12%> (-25.69%)	⬇️
tests/test_asyncio/test_commands.py	97.98% <90.47%> (-0.08%)	⬇️
redis/asyncio/client.py	93.46% <100.00%> (+0.85%)	⬆️
redis/client.py	88.59% <100.00%> (-0.96%)	⬇️
tests/test_asyncio/test_connection.py	98.06% <100.00%> (ø)
tests/test_commands.py	89.85% <100.00%> (-0.01%)	⬇️
tests/test_connection.py	98.34% <100.00%> (ø)

... and 20 files with indirect coverage changes

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

[ikonst]

related change?

[kristjanvalur]

Necessary, if we are to use the DelayProxy for the cluster tests. See tests/test_asyncio/test_cwe_404.py. In its current form it is not actually doing anything. The format is taken from the redis-py-cluster project, see PR description for more info.

[kristjanvalur]

Is there really no interest in fixing this issue properly? @ikonst, @chayim (for context, this is my many months old bugfix, which is also fixes the recently discoveed security problems with async, along with fixes to the newly added CWE tests)

[ikonst]

I'm not a redis-py maintainer so I can't do much, but can volunteer a review.

[ikonst]

Initial reaction is that with 5 discrete improvements, this is a "workspace". Also consider if 1 year from now someone would need to understand whether this PR caused a regression, it'll be hard to reason about.

Can a change like "We fix the test suite in test_cwe_404.py to be more reliable and less reliant on actual timing" be made in a separate PR that's a no-brainer to merge? (as it would clearly change only a test file and the change would be obvious)

Of course, to have the motivation to break things up, we need some commitment from maintainers (👋 @chayim) to review those separate PRs, otherwise it's more work towards no purpose.

[kristjanvalur]

I'm not a redis-py maintainer so I can't do much, but can volunteer a review.

Sorry, my mistake. I'm sometimes struggling in this project to get attention. And I find that weird given that I have been a large contributor to the async part of redis-py for the past year.

Initial reaction is that with 5 discrete improvements, this is a "workspace". Also consider if 1 year from now someone would need to understand whether this PR caused a regression, it'll be hard to reason about.

This PR is essentially started as a bugfix, to fix a regression (#2499), caused by me in pr #2104.

Keeping connections open after receiving a BaseException seemed like a good idea at the time, but I was convinced that it was more trouble than it was worth, (see discussion in #2506) and I changed my pr into essentially a revert of #2104, but with the option of keeping the connection open for special cases, like timeouts.

That's the pr. This then has lingered for months, while I keep it up-to-date.

Recently, a different incarnation of #2499 showed up. It caused a flurry of changes to the async code to be made by the maintainers, while a fix for the issue was already here, in #2506, a fix to exactly that problem

Therefore, this PR contains the following:

1. A fix for BaseException at I/O corrupts Connection #2499, which happens to be the same as for Off by 1 - Canceling async Redis command leaves connection open, in unsafe state for future commands #2624 and related issues. This is in connection.py and client.py for both sync and async.
2. It contains a unit test designed to verify the fix for Off by 1 - Canceling async Redis command leaves connection open, in unsafe state for future commands #2624.
3. It undoes the changes made in response to Off by 1 - Canceling async Redis command leaves connection open, in unsafe state for future commands #2624, which never should have been done. The changes made, using asyncio.shield() are detrimental both for code, performance and other things. These changes were in client.py and cluster.py in the async part
4. It embraces and fixes the unit-tests which were added in Off by 1 - Canceling async Redis command leaves connection open, in unsafe state for future commands #2624. They were timing related and the cluster test was not working at all.

As such, this PR is really about undoing changes, and adding unit tests. It really cannot be split into separate PRs because it only works as a whole.

I could theoretically split the fixes to the test_cwe_404.py and the addition of a remap_host_port argument to the RedisCluster off into a separate PR, but it would make this one dependent on it. we really, really, should get the old behaviour back and remove the asyncio.shield() hack from the code.

[ikonst]

I think it's fine to make this PR dependent on it. Whatever you can split off, please do.

[kristjanvalur]

See #2706 . This contains the changes to the unittest, and adds the remap feature to the RedisCluster.
Unfortunately, my fixing the unittests revealed that master is still not fixed. with regards to the problems that test_cwe_404.py was trying to verify that were fixed. Only my original fix does that.

I don't think there is any point trying to fix a unit-test which is trying to verify a fundamentally flawed fix to a problem.

What I can do, if it makes this PR more palatable to people, is to collapse all of the changes into a new set of coherent changes so that it is easier to understand what's going on. There is a lot of history here, since this PR has been open for so long.

[ikonst]

What I can do, if it makes this PR more palatable to people, is to collapse all of the changes into a new set of coherent changes so that it is easier to understand what's going on. There is a lot of history here, since this PR has been open for so long.

Given that no serious review has begun, this sounds like a win to me.

1. We fix the original regression, allowing overrides for when we do want to not terminate connection.
2. We undo changes from pr Minor fixes for #2666 and enhanced async test #2673 and related changes, bringing the client.py and cluster.py files to their original form.
3. We fix the test suite in test_cwe_404.py to be more reliable and less reliant on actual timing.
4. We add a host_port_remap feature to RedisCluster (see https://redis-py-cluster.readthedocs.io/en/stable/client.html)
5. We fixed the tests for cluster mode which were previously not doing anything because RedisCluster ignored the ports for the DelayProxy. The host_port_remap feature is used to ensure that the connections go through the DelayProxy objects.

Let's say we make a new PR with (1) only, is that possible, Or does (1) break (2)?

[ikonst]

Oh, it's not passing tests yet.

[kristjanvalur]

I"ve rebased it into three parts.

• First part is my original fix, to the regression caused by my changes, with the optional "disconnect_on_error" toggle.
• Second part undoes Minor fixes for #2666 and enhanced async test #2673 which was an attempt to solve the same issue.
• Third part is updates and fixes to the cwe_404 unit test, spelled out in individual commits for clarity

Let's say we make a new PR with (1) only, is that possible, Or does (1) break (2)?

(2) indeed breaks 1, that's how I became aware of it #2673 in the first place. It makes it impossible to cancel operations.

We don't want (2). It is not the right way to do things. asyncio.shield() is not meant for this usage. We don't want to wrap every operation in a Task (performance). We don't want to acquire and release asyncio.Lock objects in different Tasks (deadlocks). We want to be able to send tasks signals (timeouts, cancels). We don't want unexpected and asymmetric programming constructs in the code without any comment or reason. You mentioned someone looking at the code in one-year's time. Imagine coming across this:

redis-py/redis/asyncio/client.py

Lines 528 to 533 in 7fc4c76

	if self.single_connection_client:
	await self._single_conn_lock.acquire()
	return await asyncio.shield(
	self._try_send_command_parse_response(conn, *args, **options)
	)

[ikonst]

@kristjanvalur I tried reading #2666 and it wasn't immediately obvious from the PR description what's the issue it's solving (for starters, there was no issue linked).

If I understand correctly (I have only passing familiarity with asyncio) asyncio.shield suppresses propagation of cancels. Guided purely by intuition, it does does smell. Maybe even it should be reverted. But... Chesterton's Fence! — let's understand why users wanted #2666 and maybe look for a way to address that? @chayim, can shed some light on the motivation for #2666?

(Right now it's a bit "I wanted to build something else, something good, and your rotten fence was in my way", and I'm saying that as the person who started the saga with this PR. Happy to jump on a call. I really think you're putting plenty of effort into this, and want to make sure this can bear fruit.)

[kristjanvalur]

I don't think Chesterton's fence applies.
Please allow me to re-state the situation, again.

1. Last year I made a series of simplifications and changes to the async/client. One of them was to make it simpler to use subscriptions and I stumbled across code which indiscriminately closed connections when handling BaseExceptions. I changed that code in good faith, because BaseExceptions normally should be handled. This was pr Catch Exception and not BaseException in the Connection #2104
2. Later, a defect BaseException at I/O corrupts Connection #2499 was opened where it turned out that due to how the connections are pooled, this change broke behaviour. Connections might get returned to the pool in an incorrect state. Much was discussed, and I realized that I had been wrong, it is okay and prudent to close connections in this library when something happens.
3. I provided a fix to BaseException at I/O corrupts Connection #2499. This was pr Revert #2104, provide way to disable disconnects in read_response() #2506. It reverts Catch Exception and not BaseException in the Connection #2104, but adds a flag, allowing connections to stay open on error. This is to support timeouts and retries in subscriptions, which was the original reason I did the change.
4. Three months pass, and the fix does not go in.
5. Another issue shows up. It is the same issue as BaseException at I/O corrupts Connection #2499, just differently stated. There is a lot of activity and a series of experimental fixes are made, while Revert #2104, provide way to disable disconnects in read_response() #2506 just sits there, a fix to my original regression, which puts things back the way they were. No one notifies me about this, even though I've been very active in restructuring the async connection classes for the past year and would have been the obvious person to blame.
6. Finally a solution is put in place, using asyncio.shield(), And the original bug is still there. The fix is supplemented with unit tests which are buggy (there is a missing await), slow and ineffective, and it will cause deadlocks if one attempts to perform cancellations in some cases.
7. I become aware of this situation when my unittest, which is designed to trigger exactly this sort of problem with a cancel, deadlocks because of the shield() code. This was, while I was updating my PR, for the umpteenth time, to try to keep it in sync until some maintainer might feel the need to look at it.
8. I look at the unittests, realize they are broken, fix them, undo the damage done to the async code, and verify that my original PR fixes also this issue.
9. I create a new PR out of the old one to try to point out: Look, the fix was here all along! No need for any of this! and I fixed your unit tests, too, btw.

It would be nice for the maintainers to actually show interest in my efforts here. Honestly, I'm trying to help.

[ikonst]

I think @chayim should chime in at this point, as he's the author of #2666 and IIUC a maintainer.

[chayim]

Folks, apologies on delays. Life definitely has a way to get in the way.

First off @kristjanvalur Please don't think there's a lack of interest! I'm hoping you can instead view it through the lens of way too many things going on, plus holidays, plus RESP3. But you're right, it's time to pick up the ball.

Frankly, I'll be the first to admit that my async abilities are sub optimal. Regrettably, other things took priority, as happens. With the upcoming beta of 5.0.0b2 ... @dvora-h will free up a bit, and can return to contributing, reviews, etc.

Now getting back to this you're bang on the asyncio.shield change is absolutely a bandaid, in order to get things secure, and out. It's not ideal - but at the time, with what was known, I felt (perhaps incorrectly) that this was the right move forward.

Now, back to the PR. I'm a big fan of small, surgical PRs - even if this isn't always the case. To wit, I'm more than happy to rip out the unit tests test_cwe_404.py, especially once there's a full, crafted fix merged. But tl;dr I support the collapsed PR approach.

[agronholm]

This looks highly dubious; at least it should never ever be necessary here. Is this a workaround for the stdlib bug fixed in v3.11.3?

[kristjanvalur]

No, it is necessary in current Python if you decide catch and ignore a Cancel request.
I am the author of pr python/cpython#102815 which fixed the Timeout issue in 3.11.3, and among other things, that PR clarifies this use case. Please see https://github.com/python/cpython/blob/main/Doc/library/asyncio-task.rst#task-cancellation

[agronholm]

Oh, I see now. This is necessary since you're catching CancelledError above.

[dvora-h]

LGTM!
Just one small question...

[dvora-h]

Why do you add this asterisk? I prefer not to break... (if someone is using something like read_response(False, 0.5))

[kristjanvalur]

Ah, it was an afterghought. I was the one who added the timeout argument originally and in retrospect it probably should have been a keyword-only argument. Since it was my addition, I thought changing it retro-actively to a kw-only would be ok (there was only a two-month inverval between those two changes). "timeout" is traditionally a kw-only arg these days.
I guess one should be careful with these things, so I´m moving the asterisk :)

[kristjanvalur]

By the way, we could process pr #2706 first. which is a fix for the already existing tests, and then rebase this on top.

[kristjanvalur]

I've rebased this now that #2706 was merged.

[dvora-h]

Thanks @kristjanvalur !

@chayim I think this is ready for merge

[kristjanvalur]

Yay. Let's hope it fixes some of the other issues we've seen 🌵