Skip to content

Conversation

sethmlarson
Copy link
Contributor

Fixes #845. Defined InterruptedError as OSError if not defined like in Python 2.

Copy link
Contributor

@andymccurdy andymccurdy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a few small concerns, but this looks pretty good and easy to merge.

redis/_compat.py Outdated
except InterruptedError as e:
# Python 2 does not define InterruptedError, instead
# try to catch an OSError with errno == EINTR == 4.
if hasattr(e, 'errno') and (e.errno == 4 or e.errno is None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the comment helps, this could be clearer testing: e.errno == errno.EINTR. I believe the errno module is in both Py2 and Py3.

Also, in what case does e.errno is None mean we should treat the error as an interrupt and try again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be able to remove the errno is None check. I don't know if the InterruptedError exception sets errno on Python 3 but it might, we can try it! And I thought about doing the import but I'm not sure if all platforms define errno.EINTR. I know Windows has a lot of flakiness in the errno module. I'll use a getattr to be safe.

redis/_compat.py Outdated
# try to catch an OSError with errno == EINTR == 4.
if hasattr(e, 'errno') and (e.errno == 4 or e.errno is None):
continue
raise e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we just raise (without the e), we'll get the original traceback. I'm not 100% certain, but I believe on some Python versions raise e would raise the exception starting there rather than where the exception originally took place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll switch this out.

@sethmlarson
Copy link
Contributor Author

@andymccurdy Thanks for the quick review! I've implemented your review comments.

Copy link
Contributor

@andymccurdy andymccurdy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me. thanks!

@andymccurdy andymccurdy merged commit 302731f into redis:master Mar 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants