Fix issue with pack_commands returning an empty byte sequence

[jmcbailey]

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

Under certain circumstances pack_commands() adds an empty byte sequence (b"") to the returned list, which when connecting to a Redis server over SSL could cause an EOF error:

redis.exceptions.ConnectionError: Error while reading from socket: (8, 'EOF occurred in violation of protocol (_ssl.c:2483)')

In our case we are running an async Falcon application using the uvicorn worker type; we ran into the problem when using a Redis pipeline to perform an MSET followed by one or more EXPIRE commands. The values in the MSET are images, generally around 5 to 6kb in size. From what I can see it was certain images within a very specific size range, where the length
of the value was just under 6000, that caused the problem (6000 being the buffer_cutoff value used in pack_commands).

(Note that in some/many cases the b"" doesn't seem to cause a problem; I wrote a simple script to reproduce the error, executing the code in the standard asyncio event loop, but couldn't reproduce it that way. It was only by creating a simple Falcon/uvicorn app and executing it within that that I could reproduce it; I guess it's to do with the fact that uvicorn uses uvloop instead of the asyncio event loop, and there must be some difference in the underlying SSL transport.)

[chayim]

@jmcbailey Can you think of a test that can help us reproduce this?

[jmcbailey]

@jmcbailey Can you think of a test that can help us reproduce this?

@chayim I can certainly add a unit test for the method(s) to cover this scenario. Adding a test to reproduce the SSL error would be trickier...

[chayim]

@jmcbailey Does this only occur with SSL? I'm happy to forgo the SSL side if they can be isolated.

@dvora-h can you help @jmcbailey out here?

[codecov-commenter]

Codecov Report

Base: 92.28% // Head: 78.16% // Decreases project coverage by -14.13% ⚠️

Coverage data is based on head (fc84e75) compared to base (e7306aa).
Patch coverage: 100.00% of modified lines in pull request are covered.

📣 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    #2416       +/-   ##
===========================================
- Coverage   92.28%   78.16%   -14.13%     
===========================================
  Files         115      115               
  Lines       29734    29736        +2     
===========================================
- Hits        27440    23242     -4198     
- Misses       2294     6494     +4200     

Impacted Files	Coverage Δ
redis/asyncio/connection.py	80.72% <100.00%> (-6.90%)	⬇️
redis/connection.py	78.22% <100.00%> (-8.54%)	⬇️
tests/test_asyncio/test_cluster.py	18.51% <0.00%> (-79.10%)	⬇️
tests/test_cluster.py	18.00% <0.00%> (-78.76%)	⬇️
redis/asyncio/cluster.py	16.96% <0.00%> (-74.72%)	⬇️
redis/cluster.py	18.77% <0.00%> (-71.66%)	⬇️
redis/asyncio/parser.py	17.30% <0.00%> (-63.47%)	⬇️
redis/commands/cluster.py	36.95% <0.00%> (-57.07%)	⬇️
redis/crc.py	45.45% <0.00%> (-54.55%)	⬇️
tests/mocks.py	68.96% <0.00%> (-27.59%)	⬇️
... and 33 more

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.