[GROW-3247] release connection even if an unexpected exception is thrown in cluster pipeline #8

zach-iee · 2023-07-19T13:25:36Z

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

i've confirmed that the connection is not released to the connection pool, when an unexpected exception occurs during cluster pipeline execution. This PR ensures that ClusterPipeline releases connection, even if an unexpected exception occurs.

connections are disconnected before they are released to the pool in order to prevent any socket-related issues that redis-py comment discusses

…own in cluster pipeline

zach-iee · 2023-07-19T13:27:57Z

redis/cluster.py

-                        *c.args, node_flag=passed_targets
-                    )
-                    if not target_nodes:
+        try:


diff is mostly due to the added indentation. the only code i've added are try, except BaseException, n.connection = None and nodes = {}

Then i will use it

OMG never knew about this...

codecov-commenter · 2023-07-19T13:46:21Z

Codecov Report

Patch coverage: 91.25% and project coverage change: -0.02 ⚠️

Comparison is base (68c3505) 92.34% compared to head (fd76ff9) 92.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
- Coverage   92.34%   92.33%   -0.02%     
==========================================
  Files         119      119              
  Lines       30615    30638      +23     
==========================================
+ Hits        28272    28289      +17     
- Misses       2343     2349       +6

Impacted Files	Coverage Δ
redis/cluster.py	`90.86% <89.70%> (+0.11%)`	⬆️
tests/test_cluster.py	`96.76% <100.00%> (-0.19%)`	⬇️

... and 2 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.

zach-iee · 2023-07-19T14:01:54Z

redis/cluster.py

+                                n.connection = None
+                            nodes = {}


added n.connection = None, so that already returned n.connection is not used again
added nodes = {} to clear out NodeCommands from nodes

unassignement is similar to the below code
https://github.com/redis/redis-py/blob/2732a8553e58d9e77f16566b9132fc7205614a53/redis/client.py#L1181-L1183

zach-iee · 2023-07-19T14:02:04Z

redis/cluster.py

+                n.connection = None
+            nodes = {}


added n.connection = None, so that already returned n.connection is not used again
added nodes = {} to clear out NodeCommands from nodes

unassignement is similar to the below code
https://github.com/redis/redis-py/blob/2732a8553e58d9e77f16566b9132fc7205614a53/redis/client.py#L1181-L1183

zach-iee · 2023-07-19T14:02:45Z

redis/cluster.py

+        except BaseException:
+            # if nodes is not empty, a problem must have occurred
+            # since we cant guarantee the state of the connections,
+            #   disconnect before returning it to the connection pool
+            for n in nodes.values():
+                if n.connection:
+                    n.connection.disconnect()
+                    n.connection_pool.release(n.connection)
+            raise


i've added this to disconnect and return the connection to the pool

jihoShin87 · 2023-07-19T15:10:45Z

redis/cluster.py

+                            for n in nodes.values():
+                                n.connection_pool.release(n.connection)
+                                n.connection = None
+                            nodes = {}


Suggested change

nodes = {}

del nodes[node_name]

I don't think we should delete other healthy nodes. 🤔

the variable name is confusing, but nodes here aren't actual nodes in the redis cluster, but NodeCommands object that's only used in this function. Since we raise the exception in line 2051 anyway, there is no difference between del nodes[node_name] vs nodes = {}

bellatoris

for me, it completely makes sense. But beside of the changes of this PR, I can't see any reason to have infinite loop in here, because there is no continue or other paths to reach to break line except some exceptions that percolate up to out of loop (and loop is useless for this exception cases)

bellatoris · 2023-07-19T14:57:40Z

redis/cluster.py

-                        *c.args, node_flag=passed_targets
-                    )
-                    if not target_nodes:
+        try:


Then i will use it

zach-iee · 2023-07-19T15:21:49Z

for me, it completely makes sense. But beside of the changes of this PR, I can't see any reason to have infinite loop in here, because there is no continue or other paths to reach to break line except some exceptions that percolate up to out of loop (and loop is useless for this exception cases)

i completely agree. It was introduced in https://github.com/redis/redis-py/pull/2225/files, but i think continue statement got lost afterwards

jihoShin87

LGTM!!!!

…own in cluster pipeline (#8) * [GROW-3247] release connection even if an unexpected exception is thrown in cluster pipeline * [GROW-3247] fix style issue * unassign n.connection at every loop

[GROW-3247] release connection even if an unexpected exception is thr…

cc9ad30

…own in cluster pipeline

zach-iee commented Jul 19, 2023

View reviewed changes

[GROW-3247] fix style issue

85fcd53

zach-iee force-pushed the GROW-3247 branch from 8cd959e to 85fcd53 Compare July 19, 2023 13:44

unassign n.connection at every loop

fd76ff9

zach-iee commented Jul 19, 2023

View reviewed changes

zach-iee requested review from bellatoris and jihoShin87 July 19, 2023 14:51

jihoShin87 reviewed Jul 19, 2023

View reviewed changes

bellatoris reviewed Jul 19, 2023

View reviewed changes

redis/cluster.py

*c.args, node_flag=passed_targets

)

if not target_nodes:

try:

Copy link

bellatoris Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then i will use it

bellatoris approved these changes Jul 19, 2023

View reviewed changes

zach-iee requested a review from jihoShin87 July 19, 2023 15:24

jihoShin87 approved these changes Jul 20, 2023

View reviewed changes

zach-iee merged commit ae88892 into master Jul 20, 2023

zach-iee deleted the GROW-3247 branch July 25, 2023 08:53

zach-iee mentioned this pull request Aug 21, 2023

[GROW-3361] always release, instead of disconnect, when error occurs during get_connection #11

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[GROW-3247] release connection even if an unexpected exception is thrown in cluster pipeline #8

[GROW-3247] release connection even if an unexpected exception is thrown in cluster pipeline #8

Uh oh!

zach-iee commented Jul 19, 2023

Uh oh!

zach-iee Jul 19, 2023 •

edited

Loading

Uh oh!

bellatoris Jul 19, 2023

Uh oh!

zach-iee Jul 19, 2023

Uh oh!

codecov-commenter commented Jul 19, 2023 •

edited

Loading

Uh oh!

zach-iee Jul 19, 2023

Uh oh!

zach-iee Jul 19, 2023

Uh oh!

zach-iee Jul 19, 2023

Uh oh!

zach-iee Jul 19, 2023

Uh oh!

zach-iee Jul 19, 2023

Uh oh!

jihoShin87 Jul 19, 2023

Uh oh!

zach-iee Jul 19, 2023

Uh oh!

bellatoris left a comment •

edited

Loading

Uh oh!

bellatoris Jul 19, 2023

Uh oh!

zach-iee commented Jul 19, 2023

Uh oh!

jihoShin87 left a comment

Uh oh!

Uh oh!

[GROW-3247] release connection even if an unexpected exception is thrown in cluster pipeline #8

[GROW-3247] release connection even if an unexpected exception is thrown in cluster pipeline #8

Uh oh!

Conversation

zach-iee commented Jul 19, 2023

Pull Request check-list

Description of change

Uh oh!

zach-iee Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bellatoris left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zach-iee commented Jul 19, 2023

Uh oh!

jihoShin87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zach-iee Jul 19, 2023 •

edited

Loading

codecov-commenter commented Jul 19, 2023 •

edited

Loading

bellatoris left a comment •

edited

Loading