Raise OperationalError for socket timeouts #179
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Brooke-white
reviewed
Aug 4, 2023
redshift_connector/core.py
Outdated
| @@ -658,7 +658,10 @@ def get_calling_module() -> str: | |||
| self._usock.setsockopt(socket.SOL_SOCKET, socket.SO_KEEPALIVE, 1) | |||
| except socket.error as e: | |||
| self._usock.close() | |||
| raise InterfaceError("communication error", e) | |||
| if socket.timeout: | |||
There was a problem hiding this comment.
this check needs to be reworked as socket.timeout will always be true. maybe something like this?
except socket.error.timeout as timeout_e:
raise OperationalError("connection time out", timeout_e)
except socket.error as e:
raise InterfaceError("communication error", e)There was a problem hiding this comment.
Hmm, I don't believe that's true. In manual testing, this error only comes up when I either disconnect my network, or when I set timeout = 0.
There was a problem hiding this comment.
You're right.... the evaluation of if socket.timeout will always return true. I'm now using two except blocks instead as you suggested. Thank you!
jiezhen-chen
force-pushed
the
OperationalError_for_timeouts
branch
from
546588c to
d4e11c3
Compare
|
@jiezhen-chen -- testing looks good, no regressions. if you could run the repo pre-commit hooks on this change I will go ahead and merge it |
Brooke-white
approved these changes
Aug 9, 2023
soksamnanglim
pushed a commit
to soksamnanglim/amazon-redshift-python-driver
that referenced
this pull request
Aug 16, 2023
* Raise OperationalError for socket timeouts * add unit test and integrationt test * rectify unit test * rectify integration test for socket_timeout * remove empty config.ini
Closed
2 tasks
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Map socket timeout errors to OperationalError instead of InterfaceError
Description
socket.timeouts are a subclass within socket.error, and socket.error is classified as InterfaceError. Hence socket.timeouts gets classified as InterfaceError as well. This change will map socket.timeouts alone to OperationalError, where other exceptions within socket.errors will remain unchanged.
Motivation and Context
This PR resolves this issue
Testing
Conducted manual testing - hit timeout and received the below error message:
Added unit test and integration test as well.
Screenshots (if appropriate)
Types of changes
Checklist
./build.shsucceedspytest test/unitand they are passing.