Implement Thread Safe Credential Manager

[poovamraj]

Changes

Please describe both what is changing and why this is important. Include:

• Added an Executor to BaseCredentialsManager
• This executor is provided with a single-threaded ExecutorService
• getCredentials methods are run over this executor
• saveCredentials method in SecureCredentialsManager is made @Synchronized to avoid race conditions

References

#540, #330, #359

Testing

The existing Unit Tests are provided with proper mocks and fixed to accommodate current changes. Apart from that, manual testing is done where multiple threads try to access the above methods with RTR enabled

[stevehobbsdev]

Looks like it's doing the right things but needs a review from a more Kotlin/Android person as well. Reviewed the non-Code content though and left some comments 👍🏻

[adamjmcgrath]

lgtm (from what I can understand about ExecutorService)

Apart from that, manual testing is done where multiple threads try to access the above methods with RTR enabled

Can you not create a test like this one on Android?

[poovamraj]

@adamjmcgrath I checked to solve this for some time and looks like there is not a straightforward way currently to unit test a multi-threaded code in JUnit.

Although I have used Reflections (metadata information) to unit test if the saveCredentials is marked as synchronized so as to avoid accidentally removing it. I couldn't find a straightforward way for getCredentials method

[poovamraj]

@lbalmaceda One thing I would like to bring to notice is the addition of a new Executor parameter to the BaseCredentialsManager's constructor. This should be considered a breaking change, but we are planning to release this in a minor version. Would like to know if this could affect the library users much?

[lbalmaceda]

@poovamraj I see with these changes now the developer is required to pass the executor instance. I wonder if customizing that instance the "most typical use case" or if this can be set internally and so avoid having to change the constructor signatures.

[lbalmaceda]

Perhaps this breaking change can be avoided if a new/secondary constructor is defined instead keeping the old signature. For that, you'd need to have a "default executor" instance to use when one is not set by the dev.

[poovamraj]

@lbalmaceda Instead of making it a secondary constructor I have implemented it as a setter similar to _clock member. This way the user can modify it for testing purposes.

[lbalmaceda]

A few comments on this. This base abstract class is supposed to work as an interface for implementations to follow. I wouldn't add mentions to those implementations in the javadoc, as this base class "doesn't know them".

I noticed that this class is keeping the executor instance but doesn't use it at all -- It only allows to set or get the instance. To me, seems like this instance belongs somewhere else. Perhaps on the classes that implement this one, even if that's repeating that small setter piece of code (because the getter will not be required).

Also, if this is meant to be used by devs for testing purposes only, why not make this function use the "internal" modifier instead? Less chances that someone would override it on production by mistake.

[poovamraj]

Hey @lbalmaceda, I agree with you on this and exposed the executor through an internal constructor and made it private.