async fixes, remove del and other things #2870

kristjanvalur · 2023-08-02T13:54:56Z

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

__del__ handlers are rarely needed. in asyncio they should not be used to perform IO cleanup, only emit resource usage
warnings. Code executing at __del__ time can be very fragile and is best avoided.
This PR fixes those warning emitters present, making them more "shutdown" safe.

It also removes a redundant __del__ handler from the async parser. There is no need to explicitly clear member variables.
It robustifies the callback system for PubSub, eliminating the need for a __del__ handler there.
It adds a few fixes to the unittests, when run locally.

codecov-commenter · 2023-08-02T14:11:27Z

Codecov Report

Patch coverage: 77.50% and project coverage change: -13.66% ⚠️

Comparison is base (0acd0e7) 91.38% compared to head (43b3b83) 77.72%.
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    #2870       +/-   ##
===========================================
- Coverage   91.38%   77.72%   -13.66%     
===========================================
  Files         126      126               
  Lines       32469    32474        +5     
===========================================
- Hits        29671    25240     -4431     
- Misses       2798     7234     +4436

Files Changed	Coverage Δ
redis/_parsers/base.py	`90.83% <ø> (+1.23%)`	⬆️
redis/cluster.py	`18.38% <ø> (-74.43%)`	⬇️
redis/asyncio/cluster.py	`16.98% <25.00%> (-74.68%)`	⬇️
redis/asyncio/connection.py	`83.02% <80.00%> (-0.69%)`	⬇️
redis/asyncio/client.py	`92.30% <88.88%> (ø)`
redis/client.py	`91.64% <100.00%> (-0.31%)`	⬇️
redis/connection.py	`81.34% <100.00%> (-0.76%)`	⬇️
...s/test_asyncio/test_sentinel_managed_connection.py	`100.00% <100.00%> (ø)`

... and 23 files with indirect coverage changes

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

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

redis/asyncio/cluster.py

@@ -958,17 +960,18 @@ def __eq__(self, obj: Any) -> bool:

    _DEL_MESSAGE = "Unclosed ClusterNode object"

-    def __del__(self) -> None:
+    def __del__(
+        self, _warn: Any = warnings.warn, _grl: Any = asyncio.get_running_loop


Can we split these lines to too (how did black pass this!). I found it.. very difficult to find where _grl was defined - so IMHO there's an ergonomic issue here. I fully accept that this is a nit + preference.

sure. This is a pattern seen in the standard lib, the idea is to capture some methods into the function so that they are available even during module teardown. Pretty ugly.

I can't split the args, black formats it straight back to one line :(

you need to add comma after last arg to make black put them on separate lines. same for list, tuples etc. final comma splits line

chayim · 2023-09-11T06:45:02Z

redis/asyncio/connection.py


-    def clear_connect_callbacks(self):
-        self._connect_callbacks = []
+    def deregister_connect_callback(self, callback):


Given the single use, I'd argue that this isn't a breaking change - but as it's meant to be internal to the library and not reused, let's fix that here. WDYT about an underscore prefix? Yes, I could argue that register_connect_callback might have the same need :(

Done, also for non-async to keep consistency.

chayim · 2023-09-11T06:47:16Z

tests/test_asyncio/test_connection.py

-    reader = mock.AsyncMock()
-    writer = mock.AsyncMock()
-    writer.transport = mock.Mock()
+    reader = mock.Mock(spec=asyncio.StreamReader)


Teach we swami? Why is this pattern better than AsyncMock?

I think we can keep the AsyncMock the key here is the "spec". When a spec is used, they become synonymous.
The use of 'spec' is essential when mocking classes with some methods sync, ad some async, since it will create the
correct mock methods.

Oh, this went away when I rebased. But yes, when using "spec", to mock a class instance, it will create async/sync methods as appropritate. Here is the salient text:

Setting the spec of a Mock, MagicMock, or AsyncMock to a class with asynchronous and synchronous functions will automatically detect the synchronous functions and set them as MagicMock (if the parent mock is AsyncMock or MagicMock) or Mock (if the parent mock is Mock). All asynchronous functions will be AsyncMock.

chayim · 2023-09-11T06:47:39Z

tests/test_asyncio/test_cwe_404.py

        self.task.cancel()
        try:
            await self.task
        except asyncio.CancelledError:
            pass
+        await self.server.wait_closed()


definitely a better idea

chayim

@kristjanvalur I have some questions in the PR - I think they weren't sent because it was draft? Mind checking -I see them

dvora-h · 2023-09-18T23:23:59Z

@kristjanvalur Conflicts here too...

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

@kristjanvalur The CI failed

kristjanvalur · 2023-09-19T23:18:15Z

TestPubSubAutoReconnect , I guess this test could do with a review, I think there is a race condition there, causing sporadic failure.

dvora-h · 2023-09-20T07:35:24Z

@kristjanvalur conflicts again...

…ectionPool

…as for async.

kristjanvalur force-pushed the kristjan/async-cleanup branch from 6edf221 to 5b28ba7 Compare August 2, 2023 14:01

kristjanvalur marked this pull request as draft August 2, 2023 15:28

kristjanvalur force-pushed the kristjan/async-cleanup branch from 4ee679d to 04079c0 Compare August 2, 2023 16:35

kristjanvalur force-pushed the kristjan/async-cleanup branch from 04079c0 to f29a239 Compare August 25, 2023 13:05

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

chayim reviewed Sep 11, 2023

View reviewed changes

chayim reviewed Sep 14, 2023

View reviewed changes

kristjanvalur force-pushed the kristjan/async-cleanup branch 3 times, most recently from 0357301 to 044aa9d Compare September 17, 2023 12:36

dvora-h added the maintenance Maintenance (CI, Releases, etc) label Sep 18, 2023

dvora-h approved these changes Sep 18, 2023

View reviewed changes

kristjanvalur force-pushed the kristjan/async-cleanup branch from c64bdb9 to 0d735ad Compare September 19, 2023 15:51

kristjanvalur force-pushed the kristjan/async-cleanup branch from 0d735ad to 43b3b83 Compare September 19, 2023 22:52

kristjanvalur added 8 commits September 20, 2023 15:35

Use correct redis url if not default when creating Connection

08844b8

Make resource-warning __del__ code safer during shutdown

3c75ab0

Remove __del__ handler, fix pubsub weakref callback handling

6275657

Clarify comment, since there is no __del__ on asyncio.connection.Conn…

9239b08

…ectionPool

Remove remaining __del__ from async parser. They are not needed.

e5f5167

make connect callback methods internal

27e195c

similarly make non-async connect callbacks internal, use same system …

c65d043

…as for async.

Reformat __del__()

dca114d

kristjanvalur force-pushed the kristjan/async-cleanup branch from 43b3b83 to dca114d Compare September 20, 2023 15:37

dvora-h merged commit ded9f7c into redis:master Sep 20, 2023
56 checks passed

dvora-h mentioned this pull request Sep 20, 2023

Replace clear_connect_callbacks with _deregister_connect_callback #2955

Merged

6 tasks

kristjanvalur deleted the kristjan/async-cleanup branch September 21, 2023 13:26

woutdenolf mentioned this pull request Sep 27, 2023

AbstractConnection.register_connect_callback was made private in version 5.0.1 #2965

Closed

kristjanvalur mentioned this pull request Oct 5, 2023

Make the connection callback methods public again, add documentation #2980

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async fixes, remove del and other things #2870

async fixes, remove del and other things #2870

kristjanvalur commented Aug 2, 2023 •

edited

Loading

codecov-commenter commented Aug 2, 2023 •

edited

Loading

chayim Sep 11, 2023

kristjanvalur Sep 17, 2023

kristjanvalur Sep 17, 2023

Fogapod Sep 18, 2023

chayim Sep 11, 2023

kristjanvalur Sep 17, 2023

kristjanvalur Sep 17, 2023

chayim Sep 11, 2023

kristjanvalur Sep 17, 2023

kristjanvalur Sep 17, 2023

chayim Sep 11, 2023

chayim left a comment

dvora-h commented Sep 18, 2023

dvora-h commented Sep 19, 2023

kristjanvalur commented Sep 19, 2023

dvora-h commented Sep 20, 2023

async fixes, remove __del__ and other things #2870

async fixes, remove __del__ and other things #2870

Conversation

kristjanvalur commented Aug 2, 2023 • edited Loading

Pull Request check-list

Description of change

codecov-commenter commented Aug 2, 2023 • edited Loading

Codecov Report

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

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 left a comment

Choose a reason for hiding this comment

dvora-h commented Sep 18, 2023

dvora-h commented Sep 19, 2023

kristjanvalur commented Sep 19, 2023

dvora-h commented Sep 20, 2023

async fixes, remove del and other things #2870

async fixes, remove del and other things #2870

kristjanvalur commented Aug 2, 2023 •

edited

Loading

codecov-commenter commented Aug 2, 2023 •

edited

Loading