googlecodelabs / android-build-an-app-architecture-components

Other
266 stars 133 forks source link

AppExecutors Class "Race condition" #14

Open ghost opened 6 years ago

ghost commented 6 years ago

My issue addresses the part in the class named "AppExecutors", at line 47 there is a method named "getInstance", at the section : public static AppExecutors getInstance() { if (sInstance == null) { synchronized (LOCK) { sInstance = new AppExecutors(Executors.newSingleThreadExecutor(), Executors.newFixedThreadPool(3), new MainThreadExecutor()); } } return sInstance; }

This synchronized block does not actually avoid the race condition; consider the following scenario : Thread 1 & Thread 2, they both call "getInstance" method at the same exact time for the very first time, then both will evaluate "if (sInstance == null)" to be True, thus the first one to acquire the lock will instantiate "sInstance" and then the other thread will acquire the lock afterwards thus, re-instantiating the "sInstance" variable once again, which is what we were trying to avoid in the first place.

So, to only instantiate "sInstance" only exactly once no matter what, you could either swap the condition line with the synchronized line like so : synchronized (LOCK) { if (sInstance == null) { sInstance = new AppExecutors(Executors.newSingleThreadExecutor(), Executors.newFixedThreadPool(3), new MainThreadExecutor()); } }

Or to actually remove the synchronized block altogether and just make the whole function Synchronized like so : synchronized public static AppExecutors getInstance() { if (sInstance == null) { sInstance = new AppExecutors(Executors.newSingleThreadExecutor(), Executors.newFixedThreadPool(3), new MainThreadExecutor()); } return sInstance; }

This will ensure 100% that "sInstance" will only be instantiated exactly once.

Thanks for your support.