Issue 6265043: Make oauth2client support Windows-friendly locking
|
Created:
13 years, 1 month ago by cache Modified:
13 years, 1 month ago Reviewers:
jcgregorio_google CC:
google-api-python-client_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressing comments for locked_file.py #
Total comments: 9
Patch Set 3 : Further updates to locked_file.py #
Total comments: 2
Patch Set 4 : Fix broken comment. #
Total comments: 4
Patch Set 5 : Fix _Opener misname. #Patch Set 6 : Fix EACCES handling in FcntlOpener. #
Total comments: 2
Patch Set 7 : Remove unintended change to test_oauth2client_file.py #
Total comments: 8
Patch Set 8 : Update docs. #
Total comments: 4
Patch Set 9 : Safer close() handling. #MessagesTotal messages: 17
http://codereview.appspot.com/6265043/diff/1/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/1/oauth2client/locked_file.py#newc... oauth2client/locked_file.py:6: to a file, then falls back on a lock file if that is unavialable. Add an example of usage to either the module comments or the LockedFile doc comment. http://codereview.appspot.com/6265043/diff/1/oauth2client/locked_file.py#newc... oauth2client/locked_file.py:81: assert not self._locked raise an exception here instead http://codereview.appspot.com/6265043/diff/1/oauth2client/locked_file.py#newc... oauth2client/locked_file.py:92: self._fh = open(self._filename, self._mode) Note that this open might also raise an OSError exception that you are catching below. I think this needs to be moved outside the try/except. http://codereview.appspot.com/6265043/diff/1/oauth2client/locked_file.py#newc... oauth2client/locked_file.py:119: class FcntlOpener(Opener): Move the opener classes up to the module level and make them protected. Also, conditionally create _FcntlOpener only if fnctl can be imported: try: import fnctl class _FcntlOpener(Opener): ... except ImportError: _FcntlOpener = None And later you can test for a successful fnctl import with: if _FcntlOpener:
Sign in to reply to this message.
http://codereview.appspot.com/6265043/diff/1/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/1/oauth2client/locked_file.py#newc... oauth2client/locked_file.py:6: to a file, then falls back on a lock file if that is unavialable. On 2012/06/05 21:10:03, jcgregorio_google wrote: > Add an example of usage to either the module comments or the LockedFile doc > comment. Done. http://codereview.appspot.com/6265043/diff/1/oauth2client/locked_file.py#newc... oauth2client/locked_file.py:81: assert not self._locked On 2012/06/05 21:10:03, jcgregorio_google wrote: > raise an exception here instead Done. http://codereview.appspot.com/6265043/diff/1/oauth2client/locked_file.py#newc... oauth2client/locked_file.py:92: self._fh = open(self._filename, self._mode) Good catch. http://codereview.appspot.com/6265043/diff/1/oauth2client/locked_file.py#newc... oauth2client/locked_file.py:119: class FcntlOpener(Opener): On 2012/06/05 21:10:03, jcgregorio_google wrote: > Move the opener classes up to the module level and make them protected. > > Also, conditionally create _FcntlOpener only if fnctl can be imported: > > try: > import fnctl > class _FcntlOpener(Opener): > ... > except ImportError: > _FcntlOpener = None > > And later you can test for a > successful fnctl import with: > > if _FcntlOpener: Done.
Sign in to reply to this message.
http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:41: filename: The pathname of the file. Add types to all Args: parameters, i.e. filename: string, The pathname of the file. http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:113: return The bare return isn't needed. http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:168: """Represent a file that has exclusive access.""" one line before __init__ http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:198: def open_and_lock(self, timeout=10, delay=0.05): I'm not comfortable with a default timeout that isn't 0 because that is the old behavior for multistore_file.py when it used fnctl, and I don't want this to change existing program behavior.
Sign in to reply to this message.
PTAL http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:41: filename: The pathname of the file. On 2012/06/07 19:53:54, jcgregorio_google wrote: > Add types to all Args: parameters, i.e. > > filename: string, The pathname of the file. Done. http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:41: filename: The pathname of the file. On 2012/06/07 19:53:54, jcgregorio_google wrote: > Add types to all Args: parameters, i.e. > > filename: string, The pathname of the file. Done. http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:113: return On 2012/06/07 19:53:54, jcgregorio_google wrote: > The bare return isn't needed. Done. http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:168: """Represent a file that has exclusive access.""" On 2012/06/07 19:53:54, jcgregorio_google wrote: > one line before __init__ Done. http://codereview.appspot.com/6265043/diff/2002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:198: def open_and_lock(self, timeout=10, delay=0.05): On 2012/06/07 19:53:54, jcgregorio_google wrote: > I'm not comfortable with a default timeout that isn't 0 because that is the old > behavior for multistore_file.py when it used fnctl, and I don't want this to > change existing program behavior. Done.
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
http://codereview.appspot.com/6265043/diff/8002/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/8002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:135: """ Spoke too soon with the lgtm. This isn't a valid doc string.
Sign in to reply to this message.
http://codereview.appspot.com/6265043/diff/8002/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/8002/oauth2client/locked_file.py#n... oauth2client/locked_file.py:135: """ Gah. What happened there? Done.
Sign in to reply to this message.
http://codereview.appspot.com/6265043/diff/4003/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/4003/oauth2client/locked_file.py#n... oauth2client/locked_file.py:76: class _PosixOpener(Opener): _Opener http://codereview.appspot.com/6265043/diff/4003/oauth2client/locked_file.py#n... oauth2client/locked_file.py:131: class _FcntlOpener(Opener): _Opener
Sign in to reply to this message.
http://codereview.appspot.com/6265043/diff/4003/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/4003/oauth2client/locked_file.py#n... oauth2client/locked_file.py:76: class _PosixOpener(Opener): Aha. My test didn't actually work since setup.py installed the pipy version of oauth2client instead. Fixed. http://codereview.appspot.com/6265043/diff/4003/oauth2client/locked_file.py#n... oauth2client/locked_file.py:131: class _FcntlOpener(Opener): On 2012/06/11 20:39:17, jcgregorio_google wrote: > _Opener Done.
Sign in to reply to this message.
I patched this in and the tests in test_oauth2client_file.py are freezing under linux. On 2012/06/11 20:59:32, cache wrote: > http://codereview.appspot.com/6265043/diff/4003/oauth2client/locked_file.py > File oauth2client/locked_file.py (right): > > http://codereview.appspot.com/6265043/diff/4003/oauth2client/locked_file.py#n... > oauth2client/locked_file.py:76: class _PosixOpener(Opener): > Aha. My test didn't actually work since setup.py installed the pipy version of > oauth2client instead. Fixed. > > http://codereview.appspot.com/6265043/diff/4003/oauth2client/locked_file.py#n... > oauth2client/locked_file.py:131: class _FcntlOpener(Opener): > On 2012/06/11 20:39:17, jcgregorio_google wrote: > > _Opener > > Done.
Sign in to reply to this message.
Sadly it wasn't obvious how to make the tests run (I didn't have appengine-sdk), but that was a good bug. Done.
Sign in to reply to this message.
http://codereview.appspot.com/6265043/diff/2006/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/2006/oauth2client/locked_file.py#n... oauth2client/locked_file.py:146: try: This same strategy, of trying to open self._fh first before locking should also be done in _PosixOpener. http://codereview.appspot.com/6265043/diff/2007/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/2007/oauth2client/locked_file.py#n... oauth2client/locked_file.py:11: if f.is_locked(): This example isn't correct anymore, since open_and_lock() will throw an exception if the file is already locked. http://codereview.appspot.com/6265043/diff/2007/oauth2client/locked_file.py#n... oauth2client/locked_file.py:86: delay: float, How long to wait between retries. Document exceptions that could be raised. http://codereview.appspot.com/6265043/diff/2007/oauth2client/locked_file.py#n... oauth2client/locked_file.py:139: delay: float, How long to wait between retries Document exceptions that could be raised. http://codereview.appspot.com/6265043/diff/2007/oauth2client/locked_file.py#n... oauth2client/locked_file.py:223: delay: float, The number of seconds to wait between retry attempts. Document exceptions that could be raised.
Sign in to reply to this message.
http://codereview.appspot.com/6265043/diff/2006/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/2006/oauth2client/locked_file.py#n... oauth2client/locked_file.py:146: try: On 2012/06/12 12:33:34, jcgregorio_google wrote: > This same strategy, of trying to open self._fh first before locking should also > be done in _PosixOpener. Done. http://codereview.appspot.com/6265043/diff/2007/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/2007/oauth2client/locked_file.py#n... oauth2client/locked_file.py:11: if f.is_locked(): In this example the file cannot throw an example because it is already locked. Also, is_locked() is useful for knowing whether the lock succeeded or failed. http://codereview.appspot.com/6265043/diff/2007/oauth2client/locked_file.py#n... oauth2client/locked_file.py:86: delay: float, How long to wait between retries. On 2012/06/12 12:33:34, jcgregorio_google wrote: > Document exceptions that could be raised. Done. http://codereview.appspot.com/6265043/diff/2007/oauth2client/locked_file.py#n... oauth2client/locked_file.py:139: delay: float, How long to wait between retries On 2012/06/12 12:33:34, jcgregorio_google wrote: > Document exceptions that could be raised. Done. http://codereview.appspot.com/6265043/diff/2007/oauth2client/locked_file.py#n... oauth2client/locked_file.py:223: delay: float, The number of seconds to wait between retry attempts. On 2012/06/12 12:33:34, jcgregorio_google wrote: > Document exceptions that could be raised. Done.
Sign in to reply to this message.
http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py#... oauth2client/locked_file.py:56: return self._fh Define self._fh in __init__. Ran into this in local testing. http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py#... oauth2client/locked_file.py:121: self._fh.close() It may be possible that self._fh is None here, check for that before calling close(). Here and below.
Sign in to reply to this message.
http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py File oauth2client/locked_file.py (right): http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py#... oauth2client/locked_file.py:56: return self._fh On 2012/06/12 20:54:40, jcgregorio_google wrote: > Define self._fh in __init__. > > Ran into this in local testing. Done. http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py#... oauth2client/locked_file.py:121: self._fh.close() On 2012/06/12 20:54:40, jcgregorio_google wrote: > It may be possible that self._fh is None here, check for that before calling > close(). > > Here and below. Done.
Sign in to reply to this message.
lgtm On 2012/06/12 21:10:20, cache wrote: > http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py > File oauth2client/locked_file.py (right): > > http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py#... > oauth2client/locked_file.py:56: return self._fh > On 2012/06/12 20:54:40, jcgregorio_google wrote: > > Define self._fh in __init__. > > > > Ran into this in local testing. > > Done. > > http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py#... > oauth2client/locked_file.py:121: self._fh.close() > On 2012/06/12 20:54:40, jcgregorio_google wrote: > > It may be possible that self._fh is None here, check for that before calling > > close(). > > > > Here and below. > > Done.
Sign in to reply to this message.
Committed in http://code.google.com/p/google-api-python-client/source/detail?r=ac147884bf0... On 2012/06/13 16:14:32, jcgregorio_google wrote: > lgtm > > On 2012/06/12 21:10:20, cache wrote: > > http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py > > File oauth2client/locked_file.py (right): > > > > > http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py#... > > oauth2client/locked_file.py:56: return self._fh > > On 2012/06/12 20:54:40, jcgregorio_google wrote: > > > Define self._fh in __init__. > > > > > > Ran into this in local testing. > > > > Done. > > > > > http://codereview.appspot.com/6265043/diff/11003/oauth2client/locked_file.py#... > > oauth2client/locked_file.py:121: self._fh.close() > > On 2012/06/12 20:54:40, jcgregorio_google wrote: > > > It may be possible that self._fh is None here, check for that before calling > > > close(). > > > > > > Here and below. > > > > Done.
Sign in to reply to this message.
|
