Understand an abstraction before using it

Be very careful when using concurrency with abstractions

Issue

Recently, a colleague repeatedly executed an Android emulator-based test to check if the test was flaky. Out of these 100 executions, one execution had failed due to ConcurrentModificationException in the AndroidX Test library.

Since we were looking for flakiness reasons in the test and environment, it was strange to see an exception in IdlingRegistry in the AndroidX Test library. However, upon examining the exception closely, the issue turned out to be a matter of thread safety.

If you’d like, try to figure out the issue in the below Java snippet :)

Set<Resource> resources = Collections.synchronizedSet(new HashSet<Resource>);
void addResources(Collection<Resource> vals) {
    resources.addAll(Array.asList(vals));
}
Set<Resource> getResources() {
    return new HashSet<Resource>(resources);
}

Cause

At the outset, the above code looks correct as it constructs a thread-safe set resources and uses the set with no obvious thread-safety concerns at the surface.

Upon closer examination, I found the issue stemmed from how HashSet’s constructor used resources. The HashSet constructor uses an iterator to access the contents of resources. Interestingly, while all methods of SynchronizedSet synchronize on a common object, iterator() and a few other methods do not synchronize on this common object. Instead, the SynchronizedSet.iterator() method provides the iterator of the backing/underlying set and, hence, breaks thread-safety constraints.

Fix

The methods of SynchronizedSet synchronize on an object internal to the SychronizedSet instance that is not the SynchronizedSet instance or the backing/underlying set. Hence, clients that use the iterator of SynchronizedSet cannot synchronize on the internal object. While this design choice seems bizarre, I suspect there may be downsides to exposing the internal object for synchronization purposes.

Following is a simple fix to the above situation.

Set<Resource> getResources() {
    Set<Resource> result = new HashSet<Resource>();
    Collections.addAll(result, resources.toArray(new Integer[0]));
    return result;
}

Takeaways

As I wrote this post, I found such issues with synchronized collections have been discussed in the past, yet the issue surfaced in a common Android library.

Before using an abstraction (e.g., class, function) in a context, understand how the abstraction behaves in its entirety (e.g., all published methods, arguments) in all aspects (e.g., performance, concurrency) relevant in the context.

As I reported the issue to the AndroidX Test library, I realized that IdlingRegistry might not be intended for concurrent use. If this is the case, then we should document this restriction.

While creating a published abstraction, document aspect-specific restrictions pertaining to its use.

I hope you found this post useful!!