Fix race condition bug with deferred credentials #1405
Conversation
The issue occurred for the following scenario: 1) One thread secures the lock and goes through the initial refresh process 2) The thread with the lock sets the self._access_key and self._secret_key, but not yet self._frozen_credentials 3) The other threads call refresh_needed() and notice that since self._access_key and self._secret_key are set and the credential are not expired. As a result, they just return self._frozen_credentials. 4) However since self._frozen_credentials starts off as None and since self._frozen_credentials have not been set by the thread with the lock, the DeferredRefreshableCredentials returns None which causes the threads in this to raise a NoCredentialsError for those threads. To fix this, the refresh_needed() was updated to use self._frozen_credentials to check if credentials have been set.
kyleknap
requested review from
dstufft,
jamesls,
stealthycoin,
joguSD and
JordonPhillips
Codecov Report
@@ Coverage Diff @@
## develop #1405 +/- ##
========================================
Coverage 80.61% 80.61%
========================================
Files 87 87
Lines 12123 12123
========================================
Hits 9773 9773
Misses 2350 2350
Continue to review full report at Codecov.
|
|
If I'm understanding correctly the test added will only fail if we regress ~10% of the time? I'm not sure how I feel about that. |
|
So I was thinking about that too. I could maybe run this in a for loop? Increasing the thread count does not really help as I tried spinning up hundreds of threads and getting it to fail was still spotty. In general, these stress testing/race condition ones can be difficult to alarm 100% of the time and we catch these because we run the tests very often across different python versions. |
boto3 1.6.12 is the first to update its botocore dependency which in turn fixes boto/botocore#1405
Related to: aws/aws-cli#3194
The issue occurred for the following scenario:
self._access_keyandself._secret_key, but not yetself._frozen_credentialsrefresh_needed()and notice that sinceself._access_keyandself._secret_keyare set and the credential are not expired. As a result, they just returnself._frozen_credentials.self._frozen_credentialsstarts off as None and sinceself._frozen_credentialshave not been set by the thread with the lock, theDeferredRefreshableCredentialsreturns None which causes the threads in this to raise aNoCredentialsErrorfor those threads.To fix this, the
refresh_needed()was updated to useself._frozen_credentialsto check if credentials have been set.I added a test that confirmed this was an issue, and I ran it with the fix in place. I ran it in a loop (about 100 iterations) and I would see the test would fail about 10% of the time. With the fix, the test never failed no matter how many times I would try it.