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

Make connection return a thread-local instance of http. #1274

Closed
wants to merge 1 commit into from
Closed

Make connection return a thread-local instance of http. #1274

wants to merge 1 commit into from

Conversation

theacodes
Copy link
Contributor

Fixes #926, and opens up the possibility of using an object pool later.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 9, 2015
@theacodes
Copy link
Contributor Author

@dhermes this is a "basic" fix for #926. It has the drawback that a new httplib2.Http instance will be created and credentials.authorize will be called for every thread. However, I think this is preferable to just exploding when being used with multiple threads.

We can open up a new bug to create a thread-local object pool and use it to provide http objects.

dhermes
dhermes reviewed Dec 10, 2015
View reviewed changes
gcloud/connection.py
@@ -55,6 +56,8 @@ class Connection(object):
object will also need to be able to add a bearer token to API
requests and handle token refresh on 401 errors.

A custom `http` object will also need to ensure its own thread safety.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Dec 10, 2015

I think I like it but I'm scared

@theacodes theacodes mentioned this pull request Dec 11, 2015
Closed
@theacodes theacodes force-pushed the threadsafe-connection branch 2 times, most recently from 363dcda to 01bf3f5 Compare December 11, 2015 03:02
@theacodes
Copy link
Contributor Author

I think I like it but I'm scared

Agreed. Though, this is the Simplest Thing That Works™. Pooling is a better (more memory/resource friendly) way to do it, but as I said we can take small steps towards that.

@theacodes theacodes force-pushed the threadsafe-connection branch 3 times, most recently from 7daeccf to 8417e64 Compare December 11, 2015 19:01
@dhermes
Copy link
Contributor

dhermes commented Dec 14, 2015

@tseaver Can you weigh in? This will have big implications

dhermes
dhermes reviewed Dec 14, 2015
View reviewed changes
gcloud/test_connection.py
@@ -66,6 +67,26 @@ def test_user_agent_format(self):
conn = self._makeOne()
self.assertEqual(conn.USER_AGENT, expected_ua)

def test_thread_local_http(self):
credentials = _Credentials(lambda x: object())

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@theacodes theacodes force-pushed the threadsafe-connection branch 2 times, most recently from 6836a46 to 99daaaf Compare December 14, 2015 18:24
dhermes
dhermes reviewed Dec 14, 2015
View reviewed changes
gcloud/test_connection.py
self.assertEqual(len(http_objs), 2)
self.assertTrue(http_objs[0] is not http_objs[1])
self.assertTrue(http_objs[0] is not http_main)
self.assertTrue(http_objs[1] is not http_main)

This comment was marked as spam.

Sign in to view

Make connection return a thread-local instance of http.
7148ef7 
Fixes googleapis#926, and opens up the possibility of using an object pool later.
@theacodes theacodes force-pushed the threadsafe-connection branch from 99daaaf to 7148ef7 Compare December 14, 2015 18:29
@dhermes dhermes mentioned this pull request Dec 17, 2015
Merged
@theacodes
Copy link
Contributor Author

@dhermes I see there's a lot of conversation over at #1214 about making httplib2 itself threadsafe. Is this PR obsolete?

@dhermes
Copy link
Contributor

dhermes commented Jan 4, 2016

No this PR is not obsolete. We won't be fixing httplib2, I just wanted to learn how the handshake occurs.

@theacodes
Copy link
Contributor Author

Gotcha. Let me know if there's anything I need to do to move this along.

@dhermes
Copy link
Contributor

dhermes commented Jan 4, 2016

Was waiting to hear from @tseaver.

I think I'd prefer us writing a custom http implementation and using that as our default transport instead of httplib2.Http() when one is not passed.

@theacodes
Copy link
Contributor Author

I think I'd prefer us writing a custom http implementation and using that as our default transport instead of httplib2.Http() when one is not passed.

Sounds somewhat reasonable, though what sounds even more reasonable is just using urllib3. Not sure how you feel about that. Either way, can we do it with another bug/PR?

@dhermes
Copy link
Contributor

dhermes commented Jan 4, 2016

I am all for it, but we need to make oauth2client more flexible first. That's why I wanted to get 100% code coverage there. So we could re-factor it while feeling more secure that we were maintaining functionality. (googleapis/oauth2client#212)

@theacodes
Copy link
Contributor Author

SGTM. In a related effort, I've been trying to make sure urllib3 works on App Engine.

@dhermes
Copy link
Contributor

dhermes commented Jan 4, 2016

Ha! Very nice.

@bendemaree
Copy link

@jonparrott It does (mostly) though it will use urlfetch and the sandboxed sockets, IIRC. We vendor requests==2.4.0 in the python27 environment, though that's upstream of urllib3 of course, so a newer version might work as well.

Edit: urllib3==1.10 should work by my same logic. 😉 Hope this gives you some place to start from.

@dhermes dhermes mentioned this pull request Jan 5, 2016
Closed
@theacodes
Copy link
Contributor Author

@bendemaree it's now configurable, and there's a PR to requests-toolbelt to enable ongoing support for GAE.

Regardless, I think it's better to do this now, then discuss how we want to support other http libraries in another bug.

@theacodes theacodes mentioned this pull request Jan 5, 2016
Closed
@theacodes
Copy link
Contributor Author

Let's continue the discussion around the http library/transport over at #1346.

@dhermes
Copy link
Contributor

dhermes commented Jan 5, 2016

OK

@bendemaree
Copy link

@jonparrott Excellent info, thank you. 👍

@dhermes
Copy link
Contributor

dhermes commented Sep 23, 2016

@jonparrott Shall we close this PR?

@theacodes
Copy link
Contributor Author

Yeah

On Fri, Sep 23, 2016, 3:25 PM Danny Hermes [email protected] wrote:

@jonparrott https://github.com/jonparrott Shall we close this PR?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1274 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPUc4T3UKXk5TjqR833udKk1YixxCvGks5qtFHVgaJpZM4Gxj3Y
.

@dhermes dhermes closed this Sep 23, 2016
@saif-freestar
Copy link

saif-freestar commented Apr 12, 2018 •
edited
Loading

In my case..i fixed the thing with NOT using the shared connection..and downloading files in chunks async from cloud storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@theacodes @dhermes @bendemaree @saif-freestar @googlebot