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.

Sign up for GitHub

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

Jump to bottom

Fix App Engine Tunnel Host #1504

Closed
wants to merge 1 commit into from
Closed

Fix App Engine Tunnel Host #1504

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/urllib3/connection.py
View file
Open in desktop
Expand Up @@ -171,7 +171,7 @@ def _new_conn(self):

def _prepare_conn(self, conn):
self.sock = conn
if self._tunnel_host:
if getattr(self, '_tunnel_host', None):
Copy link
Member

Choose a reason for hiding this comment

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

This looks fine to me, my only question is how do our tests not catch this?
Also should we add a comment here about why we're using getattr to prevent this issue in the future?

# TODO: Fix tunnel so it doesn't depend on self.sock state.
self._tunnel()
# Mark this connection as not reusable
Expand Down Expand Up @@ -301,7 +301,8 @@ def connect(self):
conn = self._new_conn()
hostname = self.host

if self._tunnel_host:
# Google App Engine's different HTTP lib may not define_tunnel_host.
Copy link
Member

Choose a reason for hiding this comment

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

I think you need a space after "define": "define _tunnel_host".

if getattr(self, '_tunnel_host', None):
self.sock = conn
# Calls self._set_hostport(), so self.host is
# self._tunnel_host below.
Expand Down