Usage of http.DefaultClient breaks on App Engine SDK

[justonia]

For example, in dynamodb.go:98:

resp, err := http.DefaultClient.Do(hreq)

Leads to this error when using it on App Engine SDK

ERROR    2014-09-24 15:02:16,635 http_runtime.py:281] bad runtime process port

The reason for this is the default client for HTTP is purposely broken on the App Engine SDK, as they require you to use a client from the urlfetch lib they provide. Hard-coded usage of the default Go HTTP client should be removed in favor of a solution that lets you specify the HTTP client to use.

[alimoeeny]

@justonia we need a solution that works transparently for people off the app engine but has the flexibility needed for AppEngine users then. Any ideas?

[lostghost]

I believe that this could be solved with a form of dependency injection.

The dynamo package could declare an exported interface for the http client and an exported var for Client and set the value to http.DefaultClient. If a user of the package needs to replace the client for appengine, they can set the Client var to any value that satisfies the interface.

[alimoeeny]

@lostghost thanks, LGTM.
@justonia I am just curious to know, what is an example use case for calling dynamodb on appengine? I can imagine someone using S3 from appengine, but nothing for dynamo readily comes to mind. Do you have a measurement of the latencies one might expect?

[justonia]

@lostghost Yea, the solution I have seen in other libraries is to simply export a "DefaultClient" variable that the user of the library can simply override. Alternatively you could export a variable that is a factory that creates clients if you imagine needing more than one.

@alimoeeny This issue isn't specific to Dynamo, it's just the first API I was toying with via App Engine when I noticed the problem. More likely you would be interfacing with S3, SQS, or SNS. But taking the injection approach mentioned above, there would be no reason to just not fix it for all APIs. I will try and make a pull request tonight or tomorrow with the simple injection fix.

[alimoeeny]

@justonia fantastic.

[justonia]

So upon further investigation, simply having a global client variable won't cut it on AppEngine. They require all clients to be tied to the scope of a single AppEngine HTTP request (so they can track metrics, usage, etc). The consequences of this would mean that an ideal fix would be to create constructors for the various services that optionally take an http.Client to use.

This would also mean avoiding direct usage of http.Transport, which from what I can tell by browsing the source, is only used to control connection and request timeouts in two modules, aws and s3. Given that http.Client has a SetDeadline method that takes care of both of these timeouts automatically, it seems pointless to do it via the Transport.

I'm open on suggestions on how to proceed.

[alimoeeny]

@justonia so if I understand the situation correctly, what we need to make it work on AppEngine is to have AppEngine Specific constructors, right? and to make that work we need to change the way transport is being used (which if I am not mistaken, is not needed as of 1.3, I mean there are alternative ways to do the timeout etc )

[justonia]

@alimoeeny I wouldn't consider the constructors to be App Engine specific. I can envision numerous reasons why a user of the library would want to provide their own HTTP client instance to any of the services. A great example would easier mocking for this project's unit tests (meaning you can avoid having to spin up an in-process server and would no longer have the issue of being unable to run more than one suite at a time).

I wasn't aware however that the client timeout functionality was 1.3 only. What versions of Go are you guys trying to stay compatible with?

[alimoeeny]

@justonia sure, it can be more a more generic constructor.

about the go version, we are trying to stay compatible with, I can speak for myself, and that is the latest. I think it make sense to alway support one step behind. like today it will be 1.3 and 1.2 just to let give people (distros ...) catch up.

I am not quite sure if it was 1.1 -> 1.2 or 1.2->1.3 that solved the timeout and other http client "issues".