contrib.appengine.AppEngineManager #664

theacodes · 2015-06-30T19:50:55Z

This is currently very rough and early. I want to make sure I'm on the right track before continuing.

Outstanding questions:

What functionality should be covered by the connection pool? (retries, etc.)
Should I just inherit from TestConnectionPool and null out the tests that aren't applicable to GAE instead of duplicating code?
How should this connection class be wired in? On GAE Sandbox environments w/o sockets, I assume this should be default.
What exact edge cases should this class be aware; e.g, where does URLFetch behavior differ from what's expected of httplib/sockets? Do the tests already cover this?

[shazow edit: Fixes #664]

t-8ch · 2015-07-01T08:35:15Z

test/appengine/test_gae_connectionpool.py

+
+try:
+    from urllib.parse import urlencode
+except:


except ImportError?

shazow · 2015-07-01T14:44:37Z

@Lukasa @sigmavirus24 Would love your thoughts on some of the open questions inline:

What functionality should be covered by the connection pool? (retries, etc.)

Hm, I've been thinking about this. I think it may make more sense to have the AppEngine URLFetch driver be at the Manager-level (see poolmanager.py) rather than ConnectionPool-level because it doesn't make sense for our PoolManager/ProxyManager/etc to manage pools of GAEConnectionPool, right?

This should make things a fair bit more straight-forward. Any other opinions/ideas?

Should I just inherit from TestConnectionPool and null out the tests that aren't applicable to GAE instead of duplicating code?

Ideally there shouldn't be too many duplicate tests. Any overlapping functionality should be inherited (and thus won't need to be exercised separately) whereas any new functionality should be tested explicitly, right?

How should this connection class be wired in? On GAE Sandbox environments w/o sockets, I assume this should be default.

This is a good question, and it'll depend if we go with the ConnectionPool-level or Manager-level. The other question is: How automatic/implicit vs explicit should this be?

What exact edge cases should this class be aware; e.g, where does URLFetch behavior differ from what's expected of httplib/sockets? Do the tests already cover this?

Honestly I was hoping that's something you could inform us about. We're not too sure how URLFetch differs, as we don't use it ourselves. :/

Another thought: It may be better to start this off as a urllib3/contrib/ submodule. Our standards for those are somewhat lower, so it'll be easier to get this work merged and start to be used by the community while we figure out how we want to integrate it into the core API.

shazow · 2015-07-01T14:48:23Z

@jonparrott Btw, thank you again for volunteering to tackle this. We very much appreciate your time and effort. :)

Lukasa · 2015-07-01T16:30:01Z

Hm, I've been thinking about this. I think it may make more sense to have the AppEngine URLFetch driver be at the Manager-level (see poolmanager.py) rather than ConnectionPool-level because it doesn't make sense for our PoolManager/ProxyManager/etc to manage pools of GAEConnectionPool, right?

This should make things a fair bit more straight-forward. Any other opinions/ideas?

The downside of this is that GAE no longer becomes transparent to users. The ideal case would be that libraries that use urllib3 just magically work on GAE, and I don't think that'd be true if the AppEngine driver was a replacement for PoolManager instead of a replacement for ConnectionPool. Though I'm a little concern that I missed what you were aiming for here.

This is a good question, and it'll depend if we go with the ConnectionPool-level or Manager-level. The other question is: How automatic/implicit vs explicit should this be?

My vote is for automatic and implicit. The reason to do explicit is to force users to know that something different happens on GAE, but what we're doing there is pushing responsibility for testing and debugging those differences onto our users, when we should be the ones bearing that burden.

theacodes · 2015-07-01T17:14:43Z

Ideally there shouldn't be too many duplicate tests. Any overlapping functionality should be inherited (and thus won't need to be exercised separately) whereas any new functionality should be tested explicitly, right?

Cool, I'll inherit from the other test and null out the cases that aren't applicable.

Honestly I was hoping that's something you could inform us about. We're not too sure how URLFetch differs, as we don't use it ourselves. :/

I can help, absolutely, but as I mentioned before I'm not intimately familiar with how URLFetch works under the covers (though I guess I could look). Most importantly, I'd like to know what things you guys are already aware of.

It may be better to start this off as a urllib3/contrib/ submodule

Fine with me.

PoolManager vs ConnectionPool

I'm cool with it either way, it seems the implementation is basically the same (urlopen). Let me know what you guys decide.

@jonparrott Btw, thank you again for volunteering to tackle this. We very much appreciate your time and effort. :)

No worries, I'm here to help make GCP a great place for Python developers.

My vote is for automatic and implicit.

We should be aware of what we're doing to users if it's automatic and implicit, and the user should always be able to switch. Here's a comparison of App Engine's environments and the caveats I'm aware of:

_	Local Dev Server	GAE (Sandbox)	GAE (Managed VMs Compat)	GAE (Managed VMs Custom)
URLFetch	✔️	✔️	✔️	❌
Sockets	✔️	limited¹	✔️	✔️
PyOpenSSL	maybe²	optional³	optional⁴	optional⁵
Custom certs	depends⁶	depends⁶	✔️	✔️

Only paid apps, can't access google apis, etc. limitations
It's up to the user to have PyOpenSSL installed on their system.
Sandbox users have to explicitly list this, and it's a fixed, google-provided version.
Users can either use the method in 3 or modify their dockerfile / requirements to include it.
Users should be able to just add this to their requirements, but failing that, can add it to their dockerfile.
URLFetch doesn't support custom certs, but sockets + ssl do.

So, we need to figure out what the best implicit behavior is for each environment (I have code that can reliably detect this) and how to give users the ability to change it.

shazow · 2015-07-01T18:21:06Z

The downside of this is that GAE no longer becomes transparent to users. The ideal case would be that libraries that use urllib3 just magically work on GAE, and I don't think that'd be true if the AppEngine driver was a replacement for PoolManager instead of a replacement for ConnectionPool. Though I'm a little concern that I missed what you were aiming for here.

@Lukasa Why not? We could swap out PoolManager with GAEPoolManager at runtime same way we swap in VerifiedHTTPSConnectionPool. Or maybe we need to finally add another layer above PoolManager which would do this implicitly (we've been talking about adding a Session object? Or another name?).

The silly thing is that ConnectionPool objects are there to manage pools of connections (via our queue collections), which URLFetch does not have (those are managed behind the scenes by URLFetch). In effect, URLFetch is an interface to a ConnectionPool-type object. Then the PoolManager would make an instance of these ConnectionPool managers per host, which would really add nothing in terms of functionality—runtime complexity and stack depth (and possibly GAE-incompatible bugs therein).

Plus, it's not like any of our Manager-features would work with URLFetch without more interception too. For example, ProxyManager wouldn't work.

theacodes · 2015-07-01T19:42:45Z

@shazow I've moved the pool into the contrib directory.

Lukasa · 2015-07-01T20:05:14Z

Why not? We could swap out PoolManager with GAEPoolManager at runtime same way we swap in VerifiedHTTPSConnectionPool.

I suppose, but...ewww. It feels really weird to do that at runtime if we can possibly avoid it, though I suppose it's an acceptable approach.

Basically, I don't like it, but I can't articulate a good reason. Consider me -0 on that idea. =P

Lukasa · 2015-07-01T20:06:31Z

Actually, no, I think that might be fine. If it were me I'd probably take the stack depth hit, but if that's off the table, the 'patch at runtime' approach is fine.

shazow · 2015-07-01T20:37:09Z

@Lukasa Please do me a favour: sleep on it and try to articulate why you feel negative about it, I'm curious. :) Either way it's going to be a patch at runtime, right? Whether we swap the ConnectionPool or the PoolManager. Only difference is that a ConnectionPool of ConnectionPools is an extra unnecessary layer of abstraction, as far as I can tell. There's no benefit, and some potential surprises (e.g. ProxyManager not working with it).

I'd really like to get somekind of consensus on this as I think this decision will inform how we move forward in our future designs. I still suspect that adding another layer above PoolManager which automagically manages the correct things might be the best and least-surprising option.

theacodes · 2015-07-01T20:41:12Z

I still suspect that adding another layer above PoolManager which automagically manages the correct things might be the best and least-surprising option.

It's your library, but adding another layer of complexity just to support GAE's sandbox doesn't sound fun to me. If there's other use cases beyond just supporting GAE for this then I'm cool with it.

shazow · 2015-07-01T20:44:04Z

It's your library, but adding another layer of complexity just to support GAE's sandbox doesn't sound fun to me. If there's other use cases beyond just supporting GAE for this then I'm cool with it.

Yea it's something we've talked about before. urllib3 in general is largely about being very manual/explicit, but more and more we move towards autodetecting/autoconfiguring which creates a schism. Putting all that magic in one place might help keep things more sane: Things like setting up good TLS defaults (use certifi if available), proxy settings (load from .netrc/env vars), platform-specific injections (like AppEngine, or PyOpenSSL), maybe someday cookie handling (ugh).

theacodes · 2015-07-01T20:46:08Z

Gotcha, sounds good to me.

t-8ch · 2015-07-02T10:02:00Z

AFAIK URLfetch does not support chunked encoding, so when we use it implicitly streaming APIs will not work anymore.

t-8ch · 2015-07-02T10:06:52Z

urllib3/contrib/gaepool.py

+            status=urlfetch_resp.status_code
+        )
+
+    def _absolute_url(self, path):


Better move the definition from HTTPConnectionPool to ConnectionPool and use it from there

Will hold off on this until the decision of pool vs manager is settled.

Lukasa · 2015-07-02T10:14:07Z

AFAIK URLfetch does not support chunked encoding, so when we use it implicitly streaming APIs will not work anymore.

Are you serious?

From RFC 7230 Section 4.1:

A recipient MUST be able to parse and decode the chunked transfer coding.

The only way URLFetch gets around that requirement is by advertising themselves as a HTTP/1.0 client. Do they do that?

shazow · 2015-07-10T08:36:34Z

urllib3/contrib/appengine.py

+                **response_kw):
+
+        retries = self._get_retries(retries, redirect)
+        log.info(retries)


Let's try to keep logging messages consistent with other urlopens?

debugging statement I accidentally left in, fixed.

shazow · 2015-07-10T08:41:20Z

Looks sensible, just need to cleanup some debugging code and ideally match up logging messages with other pools.

Could use another pair of eyes, though. (@Lukasa @t-8ch @sigmavirus24?)

theacodes · 2015-07-10T18:42:54Z

Removed the debugging stuff (that's what I get for trying to get this pushed before I left yesterday).

shazow · 2015-07-16T09:37:30Z

@Lukasa Can you give this a quick look-over? I want to include this in the v1.11 early next week.

@jonparrott Would you be up for adding a bit of docs for how to use this module? It can live here for now: https://github.com/shazow/urllib3/blob/master/docs/contrib.rst -> https://urllib3.readthedocs.org/en/latest/contrib.html

Something along the lines of...

from urllib3 import PoolManager
from urllib3.contrib.appengine import AppEngineManager, is_appengine

# This substitution will be done automagically once appengine code
# graduates from the contrib module.
if is_appengine():
    # AppEngineManager uses AppEngine's URLFetch API behind the scenes
    http = AppEngineManager()
else:
    # PoolManager uses a socket-level API behind the scenes
    http = PoolManager()

# The client API should be consistent across managers, though some features are not available
# in URLFetch and you'll get warnings when you try to use them (like granular timeouts).
r = http.request('GET', 'https://google.com/')

Overall and example and little blurb for detecting which AppEngine environment you're in, a recipe for swapping out the respective manager, and that this will happen automagically as soon as AppEngineManager graduates from contrib-land. :)

shazow · 2015-07-16T09:41:53Z

urllib3/contrib/appengine.py

+    Connection manager for Google App Engine sandbox applications.
+
+    This manager uses the URLFetch service directly instead of using the
+    emulated httplib.


Can we add a list of features that AppEngineManager will throw PlatformError/Warnings for in the docstring?

Lukasa · 2015-07-16T17:59:33Z

@shazow I'm certainly happy to have this in contrib land for the next release.

theacodes · 2015-07-16T21:06:20Z

Fixed stuff that @Lukasa mentioned and added some docs in contrib (thanks for writing the sample for me, @shazow).

contrib: App Engine Connection Pool

shazow · 2015-07-17T08:18:51Z

@jonparrott This is fantastic and invaluable. Thank you so much for helping us out here, I'm very excited about this and the urllib3/requests community really appreciates your contribution but even moreso: I'm sure all the AppEngine users will be just ecstatic! Make sure to link to this thread in your promo packet, you really deserve some recognition for your work. :) ❤️ 🍮 ✨

theacodes · 2015-07-17T16:07:17Z

Happy to help. I'm sure this will have bugs and fun edge cases, so please mention me on any bugs that get filed on this.

webmaven · 2015-07-17T16:24:54Z

@jonparrott Thanks so much for responding when I sent up the Bat-Signal!

theacodes mentioned this pull request Jun 30, 2015

urllib3 should use URLFetch to create HTTPS connections on AppEngine #583

Closed

theacodes force-pushed the master branch 4 times, most recently from 5d24451 to 45394b5 Compare June 30, 2015 21:39

t-8ch reviewed Jul 1, 2015
View reviewed changes

theacodes force-pushed the master branch from 45394b5 to dec46fa Compare July 1, 2015 16:55

theacodes force-pushed the master branch from dec46fa to 1ec32b3 Compare July 1, 2015 17:32

theacodes force-pushed the master branch from 1ec32b3 to 4b90cd7 Compare July 1, 2015 19:41

theacodes force-pushed the master branch from 4b90cd7 to 3196a5f Compare July 1, 2015 20:09

t-8ch reviewed Jul 2, 2015
View reviewed changes

theacodes force-pushed the master branch from c49a5b6 to 9317275 Compare July 9, 2015 22:29

shazow reviewed Jul 10, 2015
View reviewed changes

theacodes force-pushed the master branch from 9317275 to 3a0b3d3 Compare July 10, 2015 18:42

shazow added this to the v1.11 milestone Jul 16, 2015

shazow reviewed Jul 16, 2015
View reviewed changes

theacodes force-pushed the master branch 2 times, most recently from 7ce31ae to 28adf54 Compare July 16, 2015 20:47

App Engine Connection Pool

86ecf0b

theacodes force-pushed the master branch from 28adf54 to 86ecf0b Compare July 16, 2015 21:05

shazow added a commit that referenced this pull request Jul 17, 2015

Merge pull request #664 from jonparrott/master

0047075

contrib: App Engine Connection Pool

shazow merged commit 0047075 into urllib3:master Jul 17, 2015

shazow changed the title ~~App Engine Connection Pool~~ contrib.appengine.AppEngineManager Jul 21, 2015

shazow added a commit that referenced this pull request Jul 21, 2015

CHANGES for #664

b8660dc

Lukasa mentioned this pull request Oct 11, 2015

Fix AppEngine support #618

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib.appengine.AppEngineManager #664

contrib.appengine.AppEngineManager #664

theacodes commented Jun 30, 2015

t-8ch Jul 1, 2015

theacodes Jul 1, 2015

shazow commented Jul 1, 2015

shazow commented Jul 1, 2015

Lukasa commented Jul 1, 2015

theacodes commented Jul 1, 2015

shazow commented Jul 1, 2015

theacodes commented Jul 1, 2015

Lukasa commented Jul 1, 2015

Lukasa commented Jul 1, 2015

shazow commented Jul 1, 2015

theacodes commented Jul 1, 2015

shazow commented Jul 1, 2015

theacodes commented Jul 1, 2015

t-8ch commented Jul 2, 2015

t-8ch Jul 2, 2015

theacodes Jul 6, 2015

Lukasa commented Jul 2, 2015

shazow Jul 10, 2015

theacodes Jul 10, 2015

shazow commented Jul 10, 2015

theacodes commented Jul 10, 2015

shazow commented Jul 16, 2015

shazow Jul 16, 2015

theacodes Jul 16, 2015

Lukasa commented Jul 16, 2015

theacodes commented Jul 16, 2015

shazow commented Jul 17, 2015

theacodes commented Jul 17, 2015

webmaven commented Jul 17, 2015

contrib.appengine.AppEngineManager #664

contrib.appengine.AppEngineManager #664

Conversation

theacodes commented Jun 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shazow commented Jul 1, 2015

shazow commented Jul 1, 2015

Lukasa commented Jul 1, 2015

theacodes commented Jul 1, 2015

shazow commented Jul 1, 2015

theacodes commented Jul 1, 2015

Lukasa commented Jul 1, 2015

Lukasa commented Jul 1, 2015

shazow commented Jul 1, 2015

theacodes commented Jul 1, 2015

shazow commented Jul 1, 2015

theacodes commented Jul 1, 2015

t-8ch commented Jul 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Jul 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shazow commented Jul 10, 2015

theacodes commented Jul 10, 2015

shazow commented Jul 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Jul 16, 2015

theacodes commented Jul 16, 2015

shazow commented Jul 17, 2015

theacodes commented Jul 17, 2015

webmaven commented Jul 17, 2015