Provide aclose() / close() for classes requiring lifetime management #2898

kristjanvalur · 2023-08-17T15:11:38Z

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

The canonical name for an asynchronous close() method in async python is aclose(). Providing this method allows support for standard programming patterns, including the use of the aclosing context manager. Conversely, a close() method should be synchronous, for the same reason.

This PR:

renames close() to aclose() method to several async classes, keeping the old close() for backwards compatibility.
renames a reset() to aclose(), keeping close() and reset() for compatibility in the asyncio.PubSub class`.
Adds an aclose() method to the async Pipeline class, and a close() method to the Pipeline class.
Adds an aclose() method to the async ConnectionPool class and the close() method to the sync ConnectionPool.

Having close() / aclose() allows the use of the closing / aclosing context managers. In particular, they can now be used to manage a ConnectionPool instance.

Note: We keep the old asynchronous close() methods as alternatives, for backwards compatibility. Maybe we should actively deprecate them?

chayim · 2023-09-11T06:26:42Z

@kristjanvalur You mentioned deprecating in your PR (and thank you). I'm 100% on board for marking as deprecated the old functions - that way that can be safely removed in future version unknown. Can you?

I might argue that we (@dvora-h) should define in our CONTRIBUTING.md or elsewhere how and when we'll deprecate an interface

chayim · 2023-09-11T06:27:15Z

docs/examples/asyncio_examples.ipynb

@@ -41,7 +41,7 @@
    "\n",
    "connection = redis.Redis()\n",
    "print(f\"Ping successful: {await connection.ping()}\")\n",
-    "await connection.close()"
+    "await connection.aclose()"


Thank you for taking the time to update the examples too!

chayim · 2023-09-11T06:27:29Z

redis/asyncio/client.py

@@ -516,6 +515,12 @@ async def close(self, close_connection_pool: Optional[bool] = None) -> None:
        ):
            await self.connection_pool.disconnect()

+    async def close(self, close_connection_pool: Optional[bool] = None) -> None:


i.e deprecating here - as yo mentioned

chayim · 2023-09-11T06:28:31Z

redis/asyncio/client.py


    def __del__(self):
        if self.connection:
            self.connection.clear_connect_callbacks()

-    async def reset(self):


This now removes a public method named reset - something we've shied away from doing. WDYT about keeping it defined, and also marking as deprecated? At the very least we should mark as breakingchange and then not merge to 5.1 otherwise.

Ah, I think you will find that I left it in place. reset() is an alias for aclose(), same as close()

If you can provide me with a guideline on how to mark methods and attributes as deprecated, and how we can schedule those for future removal, that would be awesome.

@kristjanvalur see my comment bellow about how to mark function as deprecated. for attributes, you can't use the decorator, so you just need to import warnings and warn deprecated if the user try to use this attribute, like we did here.

chayim · 2023-09-11T06:29:57Z

redis/asyncio/client.py

-        if not hasattr(self, "connection"):
-            return
-        return self.reset()
+    async def close(self) -> None:


Good catch. This never should have returned Awaitable[NoReturn].

For the curious readers see this. Note that only a function that unconditionally raises an exception should do so.

Yes, and the pattern of returning awaitables through, rather than have an intermediate "await" is not a nice one and only serves to ofuscate. I guess people think they are optimizing (removing one 'await' level) but the effect is virtually unmeaasurable.

chayim · 2023-09-11T06:30:35Z

redis/asyncio/client.py

+        """Alias for aclose(), for backwards compatibility"""
+        await self.aclose()
+
+    async def reset(self) -> None:


IMHO the None in implied. It's a nit, do you feel differently?

oh, depending on mypy warning levels, a return type is required. I have grown into the habit of always providing one explicitly.

chayim · 2023-09-11T06:30:41Z

redis/asyncio/client.py

+        await self.aclose()
+
+    async def reset(self) -> None:
+        """alias for aclose(), for backwards compatibility"""


"""Alias for aclose(), for backwards compatibility"""

chayim · 2023-09-11T06:31:26Z

tests/test_asyncio/test_pubsub.py

+            assert p.subscribed is True
+        assert p.subscribed is False
+
+    async def test_close_is_aclose(self, r: redis.Redis):


awesome, thank you ❤️

chayim · 2023-09-11T06:31:35Z

tests/test_asyncio/test_pubsub.py

+        await p.close()
+        assert p.subscribed is False
+
+    async def test_reset_is_aclose(self, r: redis.Redis):


chayim · 2023-09-11T06:31:57Z

@dvora-h WDYT? Want to take it from here? Possibly a #breakingchange

kristjanvalur · 2023-09-12T17:51:54Z

I'm 100% on board for marking as deprecated the old functions - that way that can be safely removed in future version unknown. Can you?

Sure, but what is the proper way do do that?

dvora-h · 2023-09-14T13:14:44Z

@kristjanvalur Yes, we have a deprecated_function decorator. you can add it to the function.

codecov-commenter · 2023-09-16T14:16:00Z

Codecov Report

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

Comparison is base (0acd0e7) 91.38% compared to head (09076bc) 91.36%.
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2898      +/-   ##
==========================================
- Coverage   91.38%   91.36%   -0.02%     
==========================================
  Files         126      126              
  Lines       32469    32593     +124     
==========================================
+ Hits        29671    29780     +109     
- Misses       2798     2813      +15

Files Changed	Coverage Δ
redis/cluster.py	`92.81% <ø> (ø)`
redis/commands/cluster.py	`93.61% <ø> (ø)`
tests/test_asyncio/test_commands.py	`99.72% <ø> (-0.01%)`	⬇️
tests/test_commands.py	`98.56% <ø> (-0.01%)`	⬇️
tests/test_asyncio/compat.py	`47.05% <22.22%> (-27.95%)`	⬇️
tests/test_asyncio/test_connection_pool.py	`92.02% <54.54%> (+0.07%)`	⬆️
tests/test_asyncio/test_pubsub.py	`91.73% <87.75%> (-0.55%)`	⬇️
redis/asyncio/client.py	`92.41% <90.00%> (+0.10%)`	⬆️
redis/asyncio/cluster.py	`91.70% <93.33%> (+0.03%)`	⬆️
tests/test_asyncio/test_cluster.py	`96.96% <95.00%> (+0.03%)`	⬆️
... and 8 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dvora-h · 2023-09-18T23:18:58Z

@kristjanvalur conflicts... 🙂

dvora-h · 2023-09-19T22:15:29Z

@kristjanvalur The CI failed

kristjanvalur · 2023-09-19T22:16:34Z

Bummer. Will investigate in a bit

kristjanvalur · 2023-09-19T22:38:54Z

Oops, looks like I pushed the wrong branch today! Fixing.

close() and reset() retained as aliases

kristjanvalur changed the title ~~Kristjan/aclose~~ Provide aclose() / close() for classes requiring lifetime management Aug 17, 2023

kristjanvalur force-pushed the kristjan/aclose branch 2 times, most recently from dd3e8c3 to 2bd38cc Compare August 22, 2023 16:05

kristjanvalur force-pushed the kristjan/aclose branch 2 times, most recently from 4db71b9 to 7fd7e2d Compare August 25, 2023 13:01

kristjanvalur marked this pull request as ready for review August 25, 2023 13:18

chayim added the feature New feature label Sep 11, 2023

chayim reviewed Sep 11, 2023

View reviewed changes

kristjanvalur force-pushed the kristjan/aclose branch from 7fd7e2d to c0a367f Compare September 13, 2023 15:13

kristjanvalur force-pushed the kristjan/aclose branch 2 times, most recently from 8a36bff to 79de021 Compare September 16, 2023 14:04

kristjanvalur force-pushed the kristjan/aclose branch 2 times, most recently from e3755b4 to d31e501 Compare September 17, 2023 11:23

dvora-h approved these changes Sep 18, 2023

View reviewed changes

kristjanvalur force-pushed the kristjan/aclose branch from d31e501 to e02970f Compare September 19, 2023 15:48

kristjanvalur added 14 commits September 19, 2023 22:39

Define aclose() methods instead of close() for async Redis()

158660c

update examples to use aclose()

25d29a3

Update tests to use Redis.aclose()

2a37691

Add aclose() to asyncio.client.PubSub

79e7c26

close() and reset() retained as aliases

Add aclose method to asyncio.RedisCluster

3d09daf

Add aclose() to asyncio.client.Pipeline

d7b4e7a

add close() method to sync Pipeline

bcca824

add aclose() to asyncio.connection.ConnectionPool

cba80e0

Add close() method to redis.ConnectionPool

3496e50

Deprecate older functions.

b16d3e1

changes.txt

b60dcbe

fix unittest

4b394a6

fix typo

b55d266

Update docs

09076bc

kristjanvalur force-pushed the kristjan/aclose branch from e02970f to 09076bc Compare September 19, 2023 22:41

dvora-h merged commit c46a28d into redis:master Sep 20, 2023
55 checks passed

kristjanvalur deleted the kristjan/aclose branch September 20, 2023 07:23

zware mentioned this pull request Sep 30, 2023

Update reported version deprecation in asyncio.client #2968

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide aclose() / close() for classes requiring lifetime management #2898

Provide aclose() / close() for classes requiring lifetime management #2898

kristjanvalur commented Aug 17, 2023 •

edited

Loading

chayim commented Sep 11, 2023

chayim Sep 11, 2023

chayim Sep 11, 2023

chayim Sep 11, 2023

kristjanvalur Sep 12, 2023 •

edited

Loading

dvora-h Sep 14, 2023

chayim Sep 11, 2023

kristjanvalur Sep 12, 2023

chayim Sep 11, 2023

kristjanvalur Sep 12, 2023

chayim Sep 11, 2023 •

edited

Loading

chayim Sep 11, 2023

chayim Sep 11, 2023

chayim commented Sep 11, 2023 •

edited

Loading

kristjanvalur commented Sep 12, 2023

dvora-h commented Sep 14, 2023

codecov-commenter commented Sep 16, 2023 •

edited

Loading

dvora-h commented Sep 18, 2023

dvora-h commented Sep 19, 2023

kristjanvalur commented Sep 19, 2023

kristjanvalur commented Sep 19, 2023

Provide aclose() / close() for classes requiring lifetime management #2898

Provide aclose() / close() for classes requiring lifetime management #2898

Conversation

kristjanvalur commented Aug 17, 2023 • edited Loading

Pull Request check-list

Description of change

chayim commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristjanvalur Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chayim Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chayim commented Sep 11, 2023 • edited Loading

kristjanvalur commented Sep 12, 2023

dvora-h commented Sep 14, 2023

codecov-commenter commented Sep 16, 2023 • edited Loading

Codecov Report

dvora-h commented Sep 18, 2023

dvora-h commented Sep 19, 2023

kristjanvalur commented Sep 19, 2023

kristjanvalur commented Sep 19, 2023

kristjanvalur commented Aug 17, 2023 •

edited

Loading

kristjanvalur Sep 12, 2023 •

edited

Loading

chayim Sep 11, 2023 •

edited

Loading

chayim commented Sep 11, 2023 •

edited

Loading

codecov-commenter commented Sep 16, 2023 •

edited

Loading