Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PY2 regression in connect() error handling. #150

Merged
merged 4 commits into from Nov 13, 2019
Merged

Conversation

@cglouch
Copy link
Contributor

@cglouch cglouch commented Nov 11, 2019

This autoformatting PR actually changed the behavior of the error handling:
https://github.com/httplib2/httplib2/pull/105/files#diff-c6669c781a2dee1b2d2671cab4e21c66L985

"raise socket.err, msg" is not the same as "raise socket.error(msg)" in the case where msg is an exception. For instance consider:
msg = socket.timeout("foo")
raise socket.error, msg # => socket.timeout: foo
raise socket.error(msg) # => socket.error: foo

This has the effect of potentially changing the type of the error raised by connect(). Interestingly the PY3 copy of the code doesn't have this problem (probably cause it doesn't have msg at all; not sure if it's actually needed but I figured might as well keep it for PY2 in case it is).

The change I propose here will restore the original behavior prior to the autoformat change, and will ensure that both the PY2 and PY3 copies of the code raise the same error type in the event of e.g. a socket.timeout.

This autoformatting PR actually changed the behavior of the error handling:
https://github.com/httplib2/httplib2/pull/105/files#diff-c6669c781a2dee1b2d2671cab4e21c66L985

"raise socket.err, msg" is not the same as "raise socket.error(msg)" in the case where msg is an exception. For instance consider:
msg = socket.timeout("foo")
raise socket.error, msg  # => socket.timeout: foo
raise socket.error(msg)  # => socket.error: foo

This has the effect of potentially changing the type of the error raised by connect(). Interestingly the PY3 copy of the code doesn't have this problem (probably cause it doesn't have msg at all; not sure if it's actually needed but I figured might as well keep it for PY2 in case it is).

The change I propose here will restore the original behavior prior to the autoformat change, and will ensure that both the PY2 and PY3 copies of the code raise the same error type in the event of e.g. a socket.timeout.
@codecov
Copy link

@codecov codecov bot commented Nov 11, 2019

Codecov Report

Merging #150 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   75.34%   75.51%   +0.17%     
==========================================
  Files           8        8              
  Lines        2600     2602       +2     
==========================================
+ Hits         1959     1965       +6     
+ Misses        641      637       -4
Impacted Files Coverage Δ
python2/httplib2/__init__.py 82.78% <100%> (+0.22%) ⬆️
python3/httplib2/__init__.py 83.73% <0%> (+0.2%) ⬆️

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 54ee0ef...446daf0. Read the comment docs.

@cglouch
Copy link
Contributor Author

@cglouch cglouch commented Nov 11, 2019

some admittedly hacky reproduction instructions (just monkey-patching socket.connect to raise a timeout error):

import socket
import httplib2
h = httplib2.Http(timeout=.001)
def Connect(*args, **kwargs):
  raise socket.timeout("foo")
httplib2.socket.socket.connect = Connect
h.request('http://localhost')

This will raise socket.timeout in Python 3 and in older versions of httplib2 (prior to aa1b95b), but a ResponseNotReady error in Python 2 with the latest httplib2.

@@ -1241,7 +1244,7 @@ def connect(self):
continue
break
if not self.sock:
raise socket.error(msg)
raise socket_err if socket_err else socket.error(msg)
Copy link
Member

@temoto temoto Nov 11, 2019

raise a or b would work too?

Copy link
Contributor Author

@cglouch cglouch Nov 11, 2019

even better, thanks!

@temoto
Copy link
Member

@temoto temoto commented Nov 12, 2019

There is only one assignment to msg now. Are you sure semantics didn't change?

@temoto
Copy link
Member

@temoto temoto commented Nov 12, 2019

Thanks, it's clear now.

Moved msg inline for clarity as well.
@cglouch
Copy link
Contributor Author

@cglouch cglouch commented Nov 12, 2019

Cool - I moved msg inline for clarity, and also applied an identical change to connect() in HTTPSConnectionWithTimeout since I forgot to include that originally. Let me know if anything else is needed.

@temoto temoto merged commit d12344a into httplib2:master Nov 13, 2019
3 checks passed
@temoto
Copy link
Member

@temoto temoto commented Nov 13, 2019

Thank you.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jan 6, 2020
0.15.0

  python2: regression in connect() error handling
  httplib2/httplib2#150

  add support for password protected certificate files
  httplib2/httplib2#143

  feature: Http.close() to clean persistent connections and sensitive data
  httplib2/httplib2#149
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jan 14, 2020
0.15.0

  python2: regression in connect() error handling
  httplib2/httplib2#150

  add support for password protected certificate files
  httplib2/httplib2#143

  feature: Http.close() to clean persistent connections and sensitive data
  httplib2/httplib2#149
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants