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

Magically take care of threading issues, please. #926

Closed
SurferJeffAtGoogle opened this issue Jun 17, 2015 · 27 comments
Closed

Magically take care of threading issues, please. #926

SurferJeffAtGoogle opened this issue Jun 17, 2015 · 27 comments
Assignees

Comments

@SurferJeffAtGoogle
Copy link
Contributor

I don't want to worry about connections and threads. I want to be able to pass API classes from thread to thread and have the classes take care of threading issues.

@tmatsuo
Copy link
Contributor

tmatsuo commented Jun 17, 2015

+1 I really like this idea

@dhermes dhermes added this to the Core Future milestone Jun 17, 2015
@dhermes
Copy link
Contributor

dhermes commented Jun 17, 2015

I like the idea too, but it is pretty abstract. Can you give an example where you have to deal with threading that you'd like the library to handle?

@SurferJeffAtGoogle
Copy link
Contributor Author

Yes, when I'm calling the code on app engine, my http handlers will be called from multiple threads. I can't share a connection/http object between them all because the underlying http object is not thread safe. I don't want to create a new http object for each request because then I lose the benefit of connection: keep-alive.

@SurferJeffAtGoogle
Copy link
Contributor Author

Also, I don't want to create a new http object for each request because then it has to re-authenticate.

@dhermes
Copy link
Contributor

dhermes commented Jun 17, 2015

In the short term I'd like to point to #908 and the custom code there using requests.

We'd like to address this, but if your needs are pressing, you may be happy with a short-term hack in the style described in that issue.

@tseaver
Copy link
Contributor

tseaver commented Jun 23, 2015

Given that we are moving to re-bind domain objects to clients (where clients encapsulate a connection), satisfying this goal is going to be tricky, unless the "foreign" thread is willing to pass in its client to methods which need a client.

@dhermes
Copy link
Contributor

dhermes commented Jun 23, 2015

A bigger issue may also be that httplib2 is not thread-safe. @tseaver You should check out how easily @mwitkow-io was able to make requests work in our library in a threaded environment in #908.

@theacodes
Copy link
Contributor

Pinging this issue as threadsafety for gcloud clients recently came up in discussion.

What's the status on this? Are client objects not threadsafe? If not, what needs to be done to allow that?

Also, if not, can we explicitly document that?

@dhermes
Copy link
Contributor

dhermes commented Sep 25, 2015

I think the best way to know is to write threaded code and see what happens. Anyone want to volunteer?

@theacodes
Copy link
Contributor

tentatively raises hand

I'll give a shot when I get a bit of free time.

@dhermes
Copy link
Contributor

dhermes commented Sep 25, 2015

I do know a little bit about this. For one, httplib2 is not thread safe. I have been advocating for a "divorce" from httplib2 or at least a way to give people other options (see googleapis/oauth2client#128). I feel like large structural changes should wait until googleapis/oauth2client#212 is complete though.

We already enable this with our library (#908) but it doesn't solve the auth problem for people until oauth2client catches up.

@SurferJeffAtGoogle
Copy link
Contributor Author

I coped with this issue by using thread-local instances of httplib2.Http.
https://github.com/GoogleCloudPlatform/pubsub-shout-csharp/blob/master/appengine-python-flask/pubsub.py

@dhermes
Copy link
Contributor

dhermes commented Sep 25, 2015

@SurferJeffAtGoogle It may be "faster" to avoid calling credentials = oauth2client.GoogleCredentials.get_application_default() in every single thread, but I think it's not so bad an approach.

We could manually add a connection or client pool that just binds to a thread local for the user?

@theacodes
Copy link
Contributor

We could manually add a connection or client pool that just binds to a thread local for the user?

I would be in favor of this.

@dhermes
Copy link
Contributor

dhermes commented Oct 1, 2015

@theacodes
Copy link
Contributor

I was going to recommend peeking at urllib3, but that's actually quite nice and short.

@dhermes
Copy link
Contributor

dhermes commented Oct 1, 2015

Well the long-term goal is to divorce ourselves from the transport layer in a clean way, but that also relies on googleapis/oauth2client#128 as well

@theacodes
Copy link
Contributor

Ah, sorry, I was unclear, I just meant to take urllib3's connection pooling as an example.

I like happybase's approach. Is it possible for us to more or less hide this from the user?

@dhermes
Copy link
Contributor

dhermes commented Oct 1, 2015

Yes I think we can hide it in the Client constructors.

@SurferJeffAtGoogle
Copy link
Contributor Author

I like "hiding it in the Client constructor." If we use thread locals, I don't understand the advantage of also using a pool. Is that for when a thread terminates its thread local falls back into the pool? I have no idea what the lifetime of a thread on app engine looks like.

@dhermes
Copy link
Contributor

dhermes commented Oct 1, 2015

@SurferJeffAtGoogle
Copy link
Contributor Author

Ok. That looks good.

@theacodes
Copy link
Contributor

Just a bump on this.

Today I just noticed that one of my projects assumed client was threadsafe, and proceeded to explode.

@theacodes
Copy link
Contributor

Could we not do something as trivial as have Connection.http return a context-local?

@dhermes
Copy link
Contributor

dhermes commented Dec 8, 2015

I'm teaching a course this week. I'm happy to review PRs in the evening?

@theacodes
Copy link
Contributor

I'll be traveling, but I can see what I can do next week (which is my last
week before holidays).

On Mon, Dec 7, 2015, 6:30 PM Danny Hermes [email protected] wrote:

I'm teaching a course this week. I'm happy to review PRs in the evening?


Reply to this email directly or view it on GitHub
#926 (comment)
.

theacodes pushed a commit to theacodes/google-cloud-python that referenced this issue Dec 9, 2015
Fixes googleapis#926, and opens up the possibility of using an object pool later.
theacodes pushed a commit to theacodes/google-cloud-python that referenced this issue Dec 11, 2015
Fixes googleapis#926, and opens up the possibility of using an object pool later.
theacodes pushed a commit to theacodes/google-cloud-python that referenced this issue Dec 11, 2015
Fixes googleapis#926, and opens up the possibility of using an object pool later.
theacodes pushed a commit to theacodes/google-cloud-python that referenced this issue Dec 11, 2015
Fixes googleapis#926, and opens up the possibility of using an object pool later.
theacodes pushed a commit to theacodes/google-cloud-python that referenced this issue Dec 11, 2015
Fixes googleapis#926, and opens up the possibility of using an object pool later.
theacodes pushed a commit to theacodes/google-cloud-python that referenced this issue Dec 11, 2015
Fixes googleapis#926, and opens up the possibility of using an object pool later.
theacodes pushed a commit to theacodes/google-cloud-python that referenced this issue Dec 14, 2015
Fixes googleapis#926, and opens up the possibility of using an object pool later.
theacodes pushed a commit to theacodes/google-cloud-python that referenced this issue Dec 14, 2015
Fixes googleapis#926, and opens up the possibility of using an object pool later.
theacodes pushed a commit to theacodes/google-cloud-python that referenced this issue Dec 14, 2015
Fixes googleapis#926, and opens up the possibility of using an object pool later.
@theacodes
Copy link
Contributor

Superseded by the discussion over at #1346

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants