Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Fall back to credential refresh on EDEADLK #336

Merged
merged 1 commit into from
Nov 18, 2015

Conversation

thobrla
Copy link
Contributor

@thobrla thobrla commented Nov 10, 2015

Fixes #335

This change fixes a bug where multiple threads and/or processes
using multistore_file to access the same backing store could
raise IOError errno.EDEADLK to the calling application. Since
EDEADLK is a possibility with concurrent access, this change
instead causes a fallback to read only mode and refresh
credentials if necessary.

@thobrla
Copy link
Contributor Author

thobrla commented Nov 10, 2015

Using threading.Lock / multiprocessing.Lock is substantially more complex because it's necessary to pass one per _MultistoreFile and then plumb them through. We would need this code regardless to protect against multi-application calls, so starting out with this.

Also, would like this in a release soon if that is feasible.

@dhermes
Copy link
Contributor

dhermes commented Nov 10, 2015

@thobrla Can you add tests? At some point #212 will be complete and there will be a test coverage CI test which will fail if we go below 100% line coverage

@dhermes
Copy link
Contributor

dhermes commented Nov 10, 2015

In particular, don't you have some code which caused this to fail which could be used to protect against regression for all time?

@thobrla
Copy link
Contributor Author

thobrla commented Nov 10, 2015

Added tests.


class MockLockedFile(object):

def __init__(self, filename_str, error_code):

This comment was marked as spam.

This comment was marked as spam.

@nathanielmanistaatgoogle
Copy link
Contributor

I'm pretty unfamiliar with both the problem and the solution at play here; I'd feel more comfortable with this if @dhermes also reviewed and approved this change.

@dhermes
Copy link
Contributor

dhermes commented Nov 14, 2015

FYI the test error is related to tox==2.2.1 which was released on November 11, 2015. I can confirm this happen locally as well. (I think we have a "bad" config and old versions of tox let it slide.)

The issue is caused by https://github.com/google/oauth2client/blob/311a53fbcae550526a74057f27cb9df73a0af009/tox.ini#L86-L87

for error_code in (errno.EDEADLK, errno.ENOSYS, errno.ENOLCK):
multistore = multistore_file._MultiStore(filename)
multistore._file = MockLockedFile(filename, error_code)
# Should not raise even though the underlying file class did.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Nov 14, 2015

RE: Release process. Pushing a new version is as simple as pushing a new tag. My simple process for new release

  1. Actually increment the version and update the changelog in a PR. (Releasing 1.5.0. #292 is a great example and includes a bash snippet I use to get a snapshot of all PRs merged since the previous release)
  2. Pull in all the changes after the version update PR is merged.
  3. Create the tag:
$ git tag vX.Y.Z
  1. Push the tag to GitHub
$ git push official vX.Y.Z

NOTE: I work with two remotes (origin and official):

$ git remote -vv
official        git@github.com:google/oauth2client.git (fetch)
official        git@github.com:google/oauth2client.git (push)
origin  git@github.com:dhermes/oauth2client.git (fetch)
origin  git@github.com:dhermes/oauth2client.git (push)

@nathanielmanistaatgoogle
Copy link
Contributor

Please also squash commits; there's no reason for project history to include all our review back-and-forth.

@dhermes dhermes mentioned this pull request Nov 15, 2015
@thobrla
Copy link
Contributor Author

thobrla commented Nov 16, 2015

Planning to squash commits once I have reviewer approval, or would you prefer that I squash them with each iteration?

@dhermes
Copy link
Contributor

dhermes commented Nov 16, 2015

SGTM RE: Squashing after review is completed.


def test_lock_file_raises_ioerror(self):
filehandle, filename = tempfile.mkstemp()
os.close(filehandle)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Nov 16, 2015

BTW This LGTM, Let's let @nathanielmanistaatgoogle weigh in before considering this ready to squash and merge.

@dhermes
Copy link
Contributor

dhermes commented Nov 17, 2015

@thobrla Can you rebase after #340 went in so that we can see the tests run?

@nathanielmanistaatgoogle
Copy link
Contributor

Change content LGTM; please produce a passing test run and single commit.

@dhermes please feel free to merge when satisfied.

Fixes googleapis#335

This change fixes a bug where multiple threads and/or processes
using multistore_file to access the same backing store could
raise IOError errno.EDEADLK to the calling application. Since
EDEADLK is a possibility with concurrent access, this change
instead causes a fallback to read only mode and refresh
credentials if necessary.
@dhermes
Copy link
Contributor

dhermes commented Nov 18, 2015

@thobrla Travis finally ran the tests! All green.

Ping me when you've squashed commits?

@dhermes
Copy link
Contributor

dhermes commented Nov 18, 2015

JK Just realized they were already squashed. Good call.

dhermes added a commit that referenced this pull request Nov 18, 2015
Fall back to credential refresh on EDEADLK
@dhermes dhermes merged commit 42279ca into googleapis:master Nov 18, 2015
@thobrla
Copy link
Contributor Author

thobrla commented Nov 18, 2015

Not sure why the tests took so long to run; build sat in a pending state for several hours before finally running.

@dhermes Can you pull this into a release?

@dhermes
Copy link
Contributor

dhermes commented Nov 18, 2015

Yeah Travis sometimes has issues or like doesn't like the google org / thinks it is too resource hungry. The longest we've waited for a job to run is 2.5 days, but usually it's immediate.

Can you assist with the release (send the PR with release notes and version bump as noted above)? Then I'll accept the PR / push the new tag.

@dhermes dhermes mentioned this pull request Nov 18, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multistore_file raises errno.EDEADLK when trying to open credential store
4 participants