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

Import features from ssl module with more granularity #2052

Merged
merged 1 commit into from Nov 12, 2020

Conversation

@sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Nov 12, 2020

Very subtle bug here that was caused by isort reordering the imports within try-except. Here's the diff of urllib3.util.ssl_ between 1.25.11 and 1.26.1 that caused the bug:

 try:  # Test for SSL features
     import ssl
-    from ssl import wrap_socket, CERT_REQUIRED
     from ssl import HAS_SNI  # Has SNI?
+    from ssl import CERT_REQUIRED, wrap_socket
+
+    from .ssltransport import SSLTransport
 except ImportError:
     pass

Notice how HAS_SNI is now being imported before wrap_socket and CERT_REQUIRED and HAS_SNI is a Python 2.7.9 feature. This meant that imports on Python 2.7.8 and earlier weren't getting wrap_socket imported in time before HAS_SNI was causing an ImportError.

Verified this locally with the python:2.7.8 Docker container, we currently don't have CI for this Python version.

Closes #2051

@codecov
Copy link

@codecov codecov bot commented Nov 12, 2020

Codecov Report

Merging #2052 (260a7bd) into master (6611153) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2052   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         2296      2298    +2     
=========================================
+ Hits          2296      2298    +2     
Impacted Files Coverage Δ
src/urllib3/util/ssl_.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 6611153...260a7bd. Read the comment docs.

@sethmlarson sethmlarson added this to the v1.26 milestone Nov 12, 2020
Copy link
Contributor

@nateprewitt nateprewitt left a comment

:shipit:

@sethmlarson sethmlarson merged commit b3be378 into master Nov 12, 2020
28 of 31 checks passed
@sethmlarson sethmlarson deleted the ssl-import-granularity branch Nov 12, 2020
@yaodingyd
Copy link

@yaodingyd yaodingyd commented Nov 12, 2020

appreciate the quick turnaround!

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.

3 participants