[Discussion] HTTP library

Presently, we use httplib2 by default but allow users to specify their own http library (#908) so long as it conforms to httplib2.Http's signature.

httplib2 was chosen because it's the underlying http client used by google/oauth2client. However, httplib2 has a variety of issues include not being thread-safe (#1274), not doing any connection pooling, etc.

We should consider what it would take to move to another http library. The major considerations are:

We must support using oauth2client's credentials with the library. The essential functionality is adding the auth header and performing refresh & retry. This can be done either here or within oauth2client.
The library must work on Google App Engine.

As a start, it'd be nice to

Get oauth2client to 100% code coverage (Get to 100% code coverage oauth2client#212)
Replace monkey-patching in Credentials.authorize. Idea would be that

>>> http = httplib2.Http()
>>> auth_http = credentials.authorize(http)
>>> auth_http
<oauth2client.transport.AuthenticatedHttp at 0x00deadbeaf00>

and auth_http.request would perform the same functionality that the monkey-patched stuff would

What I would prefer even more:

>>> auth_http = AuthenticatedHttp(credentials)
>>> auth_http
<oauth2client.transport.AuthenticatedHttp at 0x00deadbeaf00>

Which would allow something like:

>>> auth_session = AuthenticatedSession(credentials)
>>> auth_session
<oauth2client.transport.requests.AuthenticatedSession at 0x00deadbeaf00>

To be idiomatic.

Does it make sense to have transports be a separate library all together?

Does it make sense to have transports be a separate library all together?

nvm, the refresh logic is very oauth-centric so it doesn't quite make sense. Though I am curious what special considerations we'll need to make to use those transports in this library.

Wow, there's a ton of code in gcloud.streaming that seems to be working around httplib2.

What's the utility in continuing to support httplib2 directly? Could we move to urllib3 and be better served?

gcloud.streaming was for #935. We would have used apitools but it's future was unclear and it was (is?) completely undocumented. Hopefully we can strip down gcloud.streaming to just the core of what we need.

Your suggestion

>>> auth_http = AuthenticatedHttp(credentials)

is not agnostic of the transport. Instead you'd need

>>> auth_http = AuthenticatedHttp(credentials, http)

and you'd need a well-defined interface for http (e.g. defines http.request with signature same as httplib2.Http.request)

Sorry, I may not have been clear: AuthenticatedHttp is the transport - it's httplib2, whereas AuthenticatedSession would be a requests-based transport. Perhaps this is better:

authed_transport = transports.httplib2.Transport(credentials)
authed_transport = transports.urllib3.Transport(credentials)
authed_transport = transports.requests.Transport(credentials)

Got it. I think it'd be easier to have a simple default that can use a httplib2.Http object with no modification and then maybe have libraries like oauth2client-urllib3 and oauth2client-requests that define the necessary interface.

I'm not a fan of httplib2's interface, and I think we'd be doing ourselves a disservice if we tried to make that our definition for the future.

Personally, I'd be in favor of dropping httplib2 altogether in favor of urllib3. Requests support would then be pretty straightforward. But that's pretty extreme and the waters for urllib3 and app engine are rocky.

Now we're talking! 😀

Can you describe your desired interface?

Not sure yet. :)

Maybe something along the lines of how urllib3 does things already: the urlopen method:

    def urlopen(self, method, url, body=None, headers=None, retries=None,
                redirect=True, assert_same_host=True, timeout=_Default,
                pool_timeout=None, release_conn=None, **response_kw):

Our transport for urllib3 could just be a wrapper around urlopen that does the authentication and refresh logic. Any other transport would need to implement the same urlopen interface.

Maybe it's better to put it this way: I think it's fine to let urllib3's poolmanager define our interface. We don't necessarily need to create any abstract classes or anything like that.

Very similar to httplib2.Http.request:

def request(self, uri, method="GET", body=None, headers=None,
            redirections=DEFAULT_MAX_REDIRECTS, connection_type=None):

also google.appengine.api.urlfetch.fetch

def fetch(self, url, payload=None, method=1, headers={}, allow_truncated=False,
          follow_redirects=True, deadline=None, validate_certificate=None):

and urllib2.urlopen in 2.7.6

def urlopen(url, data=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT):

and urllib2.urlopen in 2.7.10 (latest as of Jan. 5, 2016)

def urlopen(url, data=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
            cafile=None, capath=None, cadefault=False, context=None):

We don't necessarily need to create any abstract classes or anything like that.

I agree. I think it'd be nice just to allow someone to create any old transport they like and have it work with oauth2client provided it knew how to quack.

agree. I think it'd be nice just to allow someone to create any old transport they like and have it work with oauth2client provided it knew how to quack.

That's sort of the beauty of urllib3's design - the 'Manager' is the top-level component and there's currently multiple managers (e.g, ProxyManager, AppEngineManager, etc.).

What's the current status of thread-safety in the python APIs? #1214 was closed and redirected to this issue, but it'd be nice to have a clearer explanation of what's going on in this issue with respect to thread safety. FWIW https://developers.google.com/api-client-library/python/guide/thread_safety still mentions only httplib2 and a somewhat cumbersome workaround to ensure thread safety.

@asilversempirical

Hi @asilversempirical:
@dhermes is in the process of moving us over to urllib3, which is thread-safe.
I actually thought there was an issue for this, but I can not find it, so I am reopening this one. :-)

@lukesneeringer

@lukesneeringer I'd guess that issue was #1998

Okay. Let's keep that one and close this one.

Thanks for the pointer!

chore(deps): update dependency setuptools to v63 (#1346)

chore: Remove six. (#1346)

chore(deps): update dependency certifi to v2022.6.15.1 (#1346)

feat: add integration test for universe domain (#1346)

docs: migrate code from googleapis/python-bigquery (#18303)

Navigation Menu

Repository navigation

[Discussion] HTTP library #1346

44 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[Discussion] HTTP library #1346

Description

Activity

dhermes commented on Jan 5, 2016

theacodes commented on Jan 5, 2016

theacodes commented on Jan 5, 2016

theacodes commented on Jan 5, 2016

theacodes commented on Jan 5, 2016

dhermes commented on Jan 5, 2016

theacodes commented on Jan 5, 2016

dhermes commented on Jan 5, 2016

theacodes commented on Jan 5, 2016

dhermes commented on Jan 5, 2016

theacodes commented on Jan 5, 2016

theacodes commented on Jan 5, 2016

dhermes commented on Jan 5, 2016

dhermes commented on Jan 5, 2016

theacodes commented on Jan 5, 2016

44 remaining items

asilversempirical commented on Apr 19, 2017

lukesneeringer commented on Apr 19, 2017

dhermes commented on Apr 19, 2017

lukesneeringer commented on Apr 19, 2017

asilversempirical commented on Apr 19, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions