fix socket.error raises

[x0day]

Fixes #1121

What do these changes do?

fix raise aioredis.exceptions.ConnectionError instead of raw socket.error or its subclasses

Are there changes in behavior for the user?

no

Related issue number

#1121

[eneloop2]

This kind of bug appears in more than one place so it's not actually going to address all instances of ConnectionResetError being raised.

[x0day]

@eneloop2 @Andrew-Chen-Wang
#1121 (comment)
look at this, please consider this fix, thanks.

from aioredis.connection import NONBLOCKING_EXCEPTIONS

try:
    raise ConnectionResetError
except NONBLOCKING_EXCEPTIONS:
    print('aioredis catched!')
except Exception as exc:
    print("aioredis not catched!")

from redis.connection import NONBLOCKING_EXCEPTIONS

try:
    raise ConnectionResetError
except NONBLOCKING_EXCEPTIONS:
    print('redis-py catched')

[Andrew-Chen-Wang]

@x0day do you have hiredis installed by chance?

[x0day]

@x0day do you have hiredis installed by chance?

YES, it's not parse error, just aio-redis not consider socket error as NONBLOCKING_EXCEPTIONS.

[x0day]

@seandstewart can we get this in the PR #1156 ?

[Andrew-Chen-Wang]

Thanks for the message and PR!

[Andrew-Chen-Wang]

Please add a CHANGE file though

[x0day]

@Andrew-Chen-Wang thanks for the review!

[codecov]

Codecov Report

Merging #1129 (b9cf282) into master (2ba15fb) will decrease coverage by 0.06%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master    #1129      +/-   ##
==========================================
- Coverage   90.71%   90.64%   -0.07%     
==========================================
  Files          21       21              
  Lines        6872     6875       +3     
  Branches      793      794       +1     
==========================================
- Hits         6234     6232       -2     
- Misses        467      470       +3     
- Partials      171      173       +2     

Flag	Coverage Δ
unit	90.56% <25.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aioredis/connection.py	77.15% <0.00%> (-0.52%)	⬇️
aioredis/exceptions.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ba15fb...b9cf282. Read the comment docs.

[Andrew-Chen-Wang]

One thing to note is that from redis/redis-py@8c5a41b, this piece of code was dropped due to Python 2.7 dropping. How does this resolve a Windows specific error?

[x0day]

@Andrew-Chen-Wang

FYI: at redis/redis-py@8c5a41b, it's solve the problem by raise ConnectionError instead of raw socket.error, aioredis-py's is same here.

but aioredis-py didn't catch the OSError at the Connection.read_response. it's the difference.

https://github.com/redis/redis-py/blob/791f482dcb320f48cf950c4b2c6047d1981a8f67/redis/connection.py#L753-L756
https://github.com/aio-libs/aioredis-py/blob/2ba15fb6947fa2347d401ba436e362ad62ed38ff/aioredis/connection.py#L893-L907

so if socket.error in NONBLOCKING_EXCEPTIONS, ConnectionResetError will be catched at _read_from_socket , either handle the OSError in Connection.read_response also solve the bug.

should I need to update the PR, by catching the OSError in Connection.read_response like the redis-py implement？

[Andrew-Chen-Wang]

Yes, by catching the socket.error you mean? I'm surprised BaseException doesn't catch it, but at least by catching socket.error directly we can resolve this issue + be in paralle l with redis-py.

[x0day]

@Andrew-Chen-Wang can you review the PR again? thanks.

[Andrew-Chen-Wang]

failing ci

[x0day]

@Andrew-Chen-Wang sorry for late response, failing CI because this:

https://github.com/aio-libs/aioredis-py/blob/master/aioredis/exceptions.py#L10
https://github.com/redis/redis-py/blob/9db1eec71b443b8e7e74ff503bae651dc6edf411/redis/exceptions.py#L8

aioredis-py warped builtins.ConnectionError as ConnectionError, may I known why to do this?

[Andrew-Chen-Wang]

not sure to be honest; maybe to decrease imports? If anything, LGTM