Skip to content

Conversation

kristjanvalur
Copy link
Contributor

@kristjanvalur kristjanvalur commented Dec 15, 2022

The api does not allow for a "resume", e.g. on a timeout, because an unknown number of bytes has been sent and an internal send state is not maintained. Therefore, there is no point in keeping the connection open.

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 discussion at #2560 pointed out that there is no way an open Connecition after a failed send_command() call can be useful. This PR therefore closes the Connection on every exception in that case, thus partially reverts a change made in #2104

The api does not allow for a "resume", e.g. on a timeout, because an unknown number of bytes has been sent and an internal send state is not maintained. Therefore, there is no point in keeping the connection open.
@kristjanvalur kristjanvalur force-pushed the kristjan/send_command_disconnect branch from 8825b09 to e80f72e Compare December 15, 2022 13:33
@kristjanvalur kristjanvalur marked this pull request as ready for review December 15, 2022 13:44
@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2022

Codecov Report

Base: 92.20% // Head: 92.21% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e80f72e) compared to base (74c251a).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@ Coverage Diff @@ ## master #2516 +/- ## ======================================= Coverage 92.20% 92.21% ======================================= Files 113 113 Lines 29357 29357 ======================================= + Hits 27070 27072 +2  + Misses 2287 2285 -2 
Impacted Files Coverage Δ
redis/asyncio/connection.py 86.57% <0.00%> (ø)
redis/connection.py 86.52% <0.00%> (ø)
tests/test_cluster.py 97.01% <0.00%> (+0.11%) ⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kristjanvalur
Copy link
Contributor Author

kristjanvalur commented Dec 15, 2022

Closing this in favour of #2506

@kristjanvalur kristjanvalur deleted the kristjan/send_command_disconnect branch December 15, 2022 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants