Description
Describe the feature
I see a lot of multi-threading-problems in the code. (I (also) get a headache looking at multi-threading, so I may be wrong at some points, but I definitely think it should be looked at!)
FWIW: I logged them as bug first (#3708), but @ashishdhingra considers it a feature request.
(Most of these issues have been copied to version 4 in 978251f)
DefaultInstanceProfileAWSCredentials.GetCredentials: await WriteLock if ReadLock times out
The ‘Failed-instance-error’ implies the 'local' credentials remain null. This seems to imply that the ReadLock either finds no credentials or times out and the WriteLock times out.
It seems very strange to try an exclusive WriteLock in case your non-exclusive ReadLock times out - what are the odds you will acquire a stricter lock now?
DefaultInstanceProfileAWSCredentials.GetCredentials: write in ReadLock
It seems very strange to write the _lastRetrievedCredentials within the ReadLock!
(introduced in ce982b0)
DefaultInstanceProfileAWSCredentials.GetCredentials: do not use locks, but do use volatile
It seems 'heavy' to block all requests with a ReadLock (every requests has to get credentials) for a WriteLock further down to renew credentials. Can't you just return the known credentials, even if they may be expired?
It seems possible to prevent a race condition by just setting a reference in a local variable (credentials = _lastRetrievedCredentials) and continue using that, especially if you make _lastRetrievedCredentials volatile and/or static.
DefaultInstanceProfileAWSCredentials.RenewCredentials: always refresh
Every time RenewCredentials is triggered by the timer, it will (try to) fetch new credentials, even if the present credentials are still far from expired.
This seems unnecessary.
DefaultInstanceProfileAWSCredentials.RenewCredentials: read and write outside lock
RenewCredentials will always read and write the credentials, outside the lock used by GetCredentials. (Introduced for GetCredentials by f75a608 )
DefaultInstanceProfileAWSCredentials-constructor RenewCredentials awaits thread
The DefaultInstanceProfileAWSCredentials-constructor tries to initialize the credentials by initializing the timer to run at 0 minutes. However, this is not immediate, it will have to wait for an available thread.
This means that all present requests (e.g. waiting due to an app pool recycle) may enter GetCredentials before RenewCredentials was actually called.
It might be better to try to first call it from the constructor-thread.
DefaultInstanceProfileAWSCredentials.Dispose: not disposed
It seems that DefaultInstanceProfileAWSCredentials is the only IDisposable AWSCredentials-implementor. It is instantiated by in FallbackCredentialsFactory.ContainerEC2CredentialsWrapper(), but never disposed.
Because DefaultInstanceProfileAWSCredentials seems a singleton, this probably has little consequences (but it's not very pretty either).
DefaultInstanceProfileAWSCredentials.Dispose: non volatile/static properties
I wonder if _lastRetrievedCredentials, _credentialsLock and _credentialsRetrieverTimer should/could not be volatile and/or static (but I do not think that is the cause of the current problems).
DefaultInstanceProfileAWSCredentials.GetCredentialAsync: not async (version 3.x)
GetCredentialAsync calls GetCredential in an extra Task.Run, so instead of being async (freeing thread resources), it seems to claim an extra thread?
It would be nice to have it implemented as async all the way, but as it a simple asynchronous façade does not yield any scalability benefits.
So right now, I think should have a synchronous signature for better readability/understanding.
Use Case
Every scenario that uses EC2 Instance credentials.
Proposed Solution
Make DefaultInstanceProfileAWSCredentials thread safe
Other Information
No response
Acknowledgements
- I may be able to implement this feature request
- This feature might incur a breaking change
AWS .NET SDK and/or Package version used
AWSSDK.S3 version 3.7.100.6 (implicitly using AWSSDK.Core version 3.7.100.6)
and AWSSDK.S3 Version=3.7.412.5 (implicitly using AWSSDK.Core version 3.7.401.5)
and AWSSDK.S3 Version=4.0.0.3 (implicitly using AWSSDK.Core version 4.0.0.3)
Targeted .NET Platform
.NET 8
Operating System and version
Window Server 2019 Datacenter