Add fancy locking to oauth2client

Support credential store locking in the face of high concurrency

This change consists of changing the core Storage class to support locking.  The OAuth2Credentials class is then changed to use this locking.  It will check for newer versions of a credential before refreshing.

I also fixed up some comments, style and logging.

The one 'API' change is that OAuth2Credentials.set_store now takes a Storage object instead of a put method.

[jcgregorio_google, 2011-08-25 16:03:20]

http://codereview.appspot.com/4919049/diff/3001/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/4919049/diff/3001/oauth2client/client.py#newcod...
oauth2client/client.py:188: token_uri: URI of token endpoint
two token_uris?

http://codereview.appspot.com/4919049/diff/5004/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/4919049/diff/5004/oauth2client/client.py#newcod...
oauth2client/client.py:183: token_uri: string, URI of token endpoint.
remove this token_uri desc

http://codereview.appspot.com/4919049/diff/5004/oauth2client/client.py#newcod...
oauth2client/client.py:235: has expired and been refreshed.  This implementation
use
uses

http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py
File oauth2client/locked_file.py (right):

http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py#n...
oauth2client/locked_file.py:6: credentials can be stored in one file.  That file
supports locking
I'm really uncomfortable with creating a new database, is there a reason you
didn't just use sqlite, or any of the myriad dbms that are available in the std
lib?

http://docs.python.org/library/persistence.html

http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py#n...
oauth2client/locked_file.py:11: * client_secret
client_secret is redundant

http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py#n...
oauth2client/locked_file.py:109: self._create_file_if_needed()
blank line before

[jbeda_google, 2011-08-25 16:32:07]

I've uploaded a patch to address most of this.  The biggest question is the
format of the "database".

Let me know what you think...

http://codereview.appspot.com/4919049/diff/5004/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/4919049/diff/5004/oauth2client/client.py#newcod...
oauth2client/client.py:183: token_uri: string, URI of token endpoint.
On 2011/08/25 16:03:20, jcgregorio_google wrote:
> remove this token_uri desc

Done.

http://codereview.appspot.com/4919049/diff/5004/oauth2client/client.py#newcod...
oauth2client/client.py:235: has expired and been refreshed.  This implementation
use
On 2011/08/25 16:03:20, jcgregorio_google wrote:
> uses

Done.

http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py
File oauth2client/locked_file.py (right):

http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py#n...
oauth2client/locked_file.py:6: credentials can be stored in one file.  That file
supports locking
On 2011/08/25 16:03:20, jcgregorio_google wrote:
> I'm really uncomfortable with creating a new database, is there a reason you
> didn't just use sqlite, or any of the myriad dbms that are available in the
std
> lib?
> 
> http://docs.python.org/library/persistence.html

I hadn't considered using something like sqlite or dbm.  My gut is that a "real"
database might be overkill.  Perhaps the mistake is calling it a "database".

Considerations:
* Will probably not hold >10 credentials
* Human readable is nice so users can see what sensitive stuff is in the
database.
* Path to working on windows also (dbm is UNIX only -- fcntl is unix only, but
we can work around that pretty easily here.)
* Support file locking across processes (dumbdbm doesn't do this)
* Single file so that users can blow it away easily.
* No new dependencies
* Supported back to 2.4 (sqlite3 is new in 2.5)

Let me know what you think here and if you think that one of the standard
databases fit the bill.

http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py#n...
oauth2client/locked_file.py:11: * client_secret
On 2011/08/25 16:03:20, jcgregorio_google wrote:
> client_secret is redundant

Done.

http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py#n...
oauth2client/locked_file.py:109: self._create_file_if_needed()
On 2011/08/25 16:03:20, jcgregorio_google wrote:
> blank line before 

Done.

[jcgregorio_google, 2011-08-26 14:11:18]

http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py
File oauth2client/locked_file.py (right):

http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py#n...
oauth2client/locked_file.py:6: credentials can be stored in one file.  That file
supports locking
Yeah, I think the biggest issue is using the word database, please change the
name, maybe MultipleCredentialStore, or something shorter :)

On 2011/08/25 16:32:07, jbeda wrote:
> On 2011/08/25 16:03:20, jcgregorio_google wrote:
> > I'm really uncomfortable with creating a new database, is there a reason you
> > didn't just use sqlite, or any of the myriad dbms that are available in the
> std
> > lib?
> > 
> > http://docs.python.org/library/persistence.html
> 
> I hadn't considered using something like sqlite or dbm.  My gut is that a
"real"
> database might be overkill.  Perhaps the mistake is calling it a "database".
> 
> Considerations:
> * Will probably not hold >10 credentials
> * Human readable is nice so users can see what sensitive stuff is in the
> database.
> * Path to working on windows also (dbm is UNIX only -- fcntl is unix only, but
> we can work around that pretty easily here.)
> * Support file locking across processes (dumbdbm doesn't do this)
> * Single file so that users can blow it away easily.
> * No new dependencies
> * Supported back to 2.4 (sqlite3 is new in 2.5)
> 
> Let me know what you think here and if you think that one of the standard
> databases fit the bill.

[jbeda_google, 2011-08-30 20:58:32]

On 2011/08/26 14:11:18, jcgregorio_google wrote:
> http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py
> File oauth2client/locked_file.py (right):
> 
>
http://codereview.appspot.com/4919049/diff/5004/oauth2client/locked_file.py#n...
> oauth2client/locked_file.py:6: credentials can be stored in one file.  That
file
> supports locking
> Yeah, I think the biggest issue is using the word database, please change the
> name, maybe MultipleCredentialStore, or something shorter :)
> 
> On 2011/08/25 16:32:07, jbeda wrote:
> > On 2011/08/25 16:03:20, jcgregorio_google wrote:
> > > I'm really uncomfortable with creating a new database, is there a reason
you
> > > didn't just use sqlite, or any of the myriad dbms that are available in
the
> > std
> > > lib?
> > > 
> > > http://docs.python.org/library/persistence.html
> > 
> > I hadn't considered using something like sqlite or dbm.  My gut is that a
> "real"
> > database might be overkill.  Perhaps the mistake is calling it a "database".
> > 
> > Considerations:
> > * Will probably not hold >10 credentials
> > * Human readable is nice so users can see what sensitive stuff is in the
> > database.
> > * Path to working on windows also (dbm is UNIX only -- fcntl is unix only,
but
> > we can work around that pretty easily here.)
> > * Support file locking across processes (dumbdbm doesn't do this)
> > * Single file so that users can blow it away easily.
> > * No new dependencies
> > * Supported back to 2.4 (sqlite3 is new in 2.5)
> > 
> > Let me know what you think here and if you think that one of the standard
> > databases fit the bill.

Please take another look.  Hopefully we are getting close!

[jcgregorio_google, 2011-08-31 18:06:55]

Yes, we are close, just two more minor issues.

http://codereview.appspot.com/4919049/diff/20001/oauth2client/tools.py
File oauth2client/tools.py (right):

http://codereview.appspot.com/4919049/diff/20001/oauth2client/tools.py#newcod...
oauth2client/tools.py:115: httpd =
BaseHTTPServer.HTTPServer((FLAGS.auth_host_name, port),
You didn't write this, but I just noticed that BaseHTTPServer.HTTPServer above
should be ClientRedirectServer.

http://codereview.appspot.com/4919049/diff/20001/oauth2client/tools.py#newcod...
oauth2client/tools.py:142: raise UserAuthorizationError('Authentication request
was rejected.')
I'm not sure this is a good user experience for the samples, which is where
tools.run is primarily used from. The sample apps should print messages but we
shouldn't dump them with an stack trace. Same below where you removed the try:.

[jbeda_google, 2011-09-06 23:10:18]

Please take another look.  Hopefully we are there!

http://codereview.appspot.com/4919049/diff/20001/oauth2client/tools.py
File oauth2client/tools.py (right):

http://codereview.appspot.com/4919049/diff/20001/oauth2client/tools.py#newcod...
oauth2client/tools.py:115: httpd =
BaseHTTPServer.HTTPServer((FLAGS.auth_host_name, port),
On 2011/08/31 18:06:55, jcgregorio_google wrote:
> You didn't write this, but I just noticed that BaseHTTPServer.HTTPServer above
> should be ClientRedirectServer.

Done.

http://codereview.appspot.com/4919049/diff/20001/oauth2client/tools.py#newcod...
oauth2client/tools.py:142: raise UserAuthorizationError('Authentication request
was rejected.')
On 2011/08/31 18:06:55, jcgregorio_google wrote:
> I'm not sure this is a good user experience for the samples, which is where
> tools.run is primarily used from. The sample apps should print messages but we
> shouldn't dump them with an stack trace. Same below where you removed the
try:.

I hear you -- I can revert to the old behavior but I think this code is more
useful if we throw.  I guess that updating all of the samples to install a
reasonable default exception handler is probably asking too much.

The only reason I had this behavior is so that in our debug mode I can log an
exception with a pointer to the source of the error.  I guess I can give that
up.

[jcgregorio_google, 2011-09-08 15:56:06]

http://codereview.appspot.com/4919049/diff/20001/oauth2client/tools.py
File oauth2client/tools.py (right):

http://codereview.appspot.com/4919049/diff/20001/oauth2client/tools.py#newcod...
oauth2client/tools.py:142: raise UserAuthorizationError('Authentication request
was rejected.')
On 2011/09/06 23:10:18, jbeda wrote:
> On 2011/08/31 18:06:55, jcgregorio_google wrote:
> > I'm not sure this is a good user experience for the samples, which is where
> > tools.run is primarily used from. The sample apps should print messages but
we
> > shouldn't dump them with an stack trace. Same below where you removed the
> try:.
> 
> I hear you -- I can revert to the old behavior but I think this code is more
> useful if we throw.  I guess that updating all of the samples to install a
> reasonable default exception handler is probably asking too much.

Right, tools.run() was written explicitly for command-line applications. Maybe
create a method that does the work of run and throws exceptions and then change
run() to use that function and catch the exceptions and covert them into user
friendly messages on stdout.

> 
> The only reason I had this behavior is so that in our debug mode I can log an
> exception with a pointer to the source of the error.  I guess I can give that
> up.

[jbeda_google, 2011-09-09 18:09:57]

I'm cool with it the way it was.  If I end up *really* needing a
'run_with_exceptions' method we can add that later.  I was going to write
that but the lack of an obvious good name gave me pause. :)

I think this change should be good to go.  Let me know if you need anything
else.

Joe

On Thu, Sep 8, 2011 at 8:56 AM, <jcgregorio@google.com> wrote:

>
>
http://codereview.appspot.com/**4919049/diff/20001/**oauth2client/tools.py<ht...
> File oauth2client/tools.py (right):
>
> http://codereview.appspot.com/**4919049/diff/20001/**
>
oauth2client/tools.py#**newcode142<http://codereview.appspot.com/4919049/diff/20001/oauth2client/tools.py#newcode142>
> oauth2client/tools.py:142: raise UserAuthorizationError('**Authentication
> request was rejected.')
> On 2011/09/06 23:10:18, jbeda wrote:
>
>> On 2011/08/31 18:06:55, jcgregorio_google wrote:
>> > I'm not sure this is a good user experience for the samples, which
>>
> is where
>
>> > tools.run is primarily used from. The sample apps should print
>>
> messages but we
>
>> > shouldn't dump them with an stack trace. Same below where you
>>
> removed the
>
>> try:.
>>
>
>  I hear you -- I can revert to the old behavior but I think this code
>>
> is more
>
>> useful if we throw.  I guess that updating all of the samples to
>>
> install a
>
>> reasonable default exception handler is probably asking too much.
>>
>
> Right, tools.run() was written explicitly for command-line applications.
> Maybe create a method that does the work of run and throws exceptions
> and then change run() to use that function and catch the exceptions and
> covert them into user friendly messages on stdout.
>
>
>
>  The only reason I had this behavior is so that in our debug mode I can
>>
> log an
>
>> exception with a pointer to the source of the error.  I guess I can
>>
> give that
>
>> up.
>>
>
>
http://codereview.appspot.com/**4919049/<http://codereview.appspot.com/4919049/>
>

[jcgregorio_google, 2011-09-09 18:46:42]

LGTM

[jcgregorio_google, 2011-09-09 18:53:33]

I spoke too soon. When running the unit tests I now get a failure:



python runtests.py tests
Running the tests found in the following modules:
['tests.test_http', 'tests.test_errors', 'tests.test_mocks', 'tests.test_model',
'tests.test_json_model', 'tests.test_oauth', 'tests.test_protobuf_model',
'tests.test_oauth2client_appengine', 'tests.test_discovery',
'tests.test_oauth2client']
...............................E.........................................
======================================================================
ERROR: test_required (tests.test_oauth2client_appengine.DecoratorTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File
"/usr/local/google/home/jcgregorio/projects/apiclient/tests/test_oauth2client_appengine.py",
line 195, in test_required
    self.decorator.credentials.store(self.decorator.credentials)
TypeError: 'StorageByKeyName' object is not callable

----------------------------------------------------------------------
Ran 73 tests in 0.620s

[jbeda_google, 2011-09-09 19:25:29]

Ah!  I ran the tests but couldn't figure out how to run the appengine test.
 What do I need to do to run that one?

Joe

On Fri, Sep 9, 2011 at 11:53 AM, <jcgregorio@google.com> wrote:

> I spoke too soon. When running the unit tests I now get a failure:
>
>
>
> python runtests.py tests
> Running the tests found in the following modules:
> ['tests.test_http', 'tests.test_errors', 'tests.test_mocks',
> 'tests.test_model', 'tests.test_json_model', 'tests.test_oauth',
> 'tests.test_protobuf_model', 'tests.test_oauth2client_**appengine',
> 'tests.test_discovery', 'tests.test_oauth2client']
> ..............................**.E............................**
> .............
> ==============================**==============================**==========
> ERROR: test_required (tests.test_oauth2client_**appengine.DecoratorTests)
> ------------------------------**------------------------------**----------
> Traceback (most recent call last):
>  File
> "/usr/local/google/home/**jcgregorio/projects/apiclient/**
> tests/test_oauth2client_**appengine.py",
> line 195, in test_required
>    self.decorator.credentials.**store(self.decorator.**credentials)
> TypeError: 'StorageByKeyName' object is not callable
>
> ------------------------------**------------------------------**----------
> Ran 73 tests in 0.620s
>
>
>
>
http://codereview.appspot.com/**4919049/<http://codereview.appspot.com/4919049/>
>

[jbeda_google, 2011-09-10 15:18:29]

Unit test fixed! PTAL and hopefully we are good to go.

[jcgregorio_google, 2011-09-11 18:05:45]

LGTM 

Committed at:

http://code.google.com/p/google-api-python-client/source/detail?r=c4c1be2a741...