Blog Infos
Author
Published
Topics
Published
Topics

I’ve a very long history of refactoring the code whenever I see the the yellow marks in the right gutter of code editor window in Android Studio. The best part of having an intelligent IDE is that along with showing the issue it also suggests a fix which can be applied directly (wherever possible). Like in the below screenshot you can see Remove redundant ‘public’ modifier option to apply the fix.

 

Android Studio (should be same for Intellij IDEs) showing and suggesting a fix for the warning

 

I’ve used this feature very frequently to do some minor refactoring since many years but recently doing something similar I ended up shooting myself in the foot 😅.

EDIT: After getting some feedback/comments I realized that my wordings might be causing readers think that I’m blaming Android Studio for this bug 😅, but that’s not the case. I’m not at all blaming Android Studio for this as it was showing me correct suggestion so that I can save some memory by not using index based Iterator. The actual issue was on my end that I should’ve checked the final suggestion applied code and verified that if the overall behaviour has not been changed.

The Code Refactor

Let’s start with some background first. As you can see in the below code we have a list of observers which we’ll be using to notify the classes which implements it.

interface Observer {
fun onStatusChange()
}
class ObserverImpl: Observer {
override fun onStatusChange() {
// do some operation
}
}
val observers = mutableListOf<Observer>()

Redacted code for Observer loop crash

 

And the observers list was being used by another class like something below:

for (i in 0 until observers.size) {
observers[i].onStatusChange()
}

Looking at the code we can see it’s very straightforward where we’re creating a range and iterating through it and calling onStatusChange() function. When I was adding some functionality to the same class which Android Studio showed me that this can be refactored to something like below:

 

Android Studio suggesting a fix for more optimized for loop

 

Suggested refactor also makes sense as using the until operator we are essentially allotting some memory for index (i.e. i) which is not required apart from accessing the element. So I applied the suggested change and the code got changed as shown below:

for (observer in observers) {
observer.onStatusChange()
}

After Observer loop crash

 

The Crash

Everything was good till the above code refactor got pushed in the release build. But after few days of the release when I opened Crashlytics to see if there are any new crashes being reported and I got surprised. I saw a java.util.ConcurrentModificationException crash being reported at the same location where I refactored looping code earlier 😕.

ConcurrentModificationException Exception

I looked at the both variants of code by putting them side by side and at first glance it seemed similar, as in both the cases we’re using Kotlin’s in (i.e. iterator operator) 🤔. Then as we all know that Kotlin compiler is smart and infers the type of the object, I thought about checking the Iterator type being used for in both the cases.

The RCA

In the first case where in was used with IntRangeIntProgressionIterator gets used to do the iteration. If we check the code for IntProgressionIterator we can see that there is no logic to throw ConcurrentModificationException. This makes sense as once the range is created there is no way to modify it.

in interator operator resolves to IntProgression.iterator() (i.e. IntProgressionIterator) for IntRanges

IntProgressionIterator source code

Job Offers

Job Offers

There are currently no vacancies.

OUR VIDEO RECOMMENDATION

Jobs

Now let’s check the second case where in is used with a List. In this case we end up using the Iterator

 

in interator operator resolves to List.iterator() for List/ArrayList

 

ListIterator code with ConcurrentModificationException being thrown at line 860

 

As we know in our case observers list was a MutableList (which is backed by an ArrayList internally) I opened ArrayList file. Also looking at the Crashlytics stacktrace shown earlier we can see that the crash gets triggered on ArrayList.java:860. So I directly navigated to that line and saw ConcurrentModificationException being thrown in case modCount != expectedModCount, which can happen if your mutable list gets modified by some other thread when it was being iterated over. And looking at the actual code in my codebase I realized that very well may happen 😅.

The Fix and Retrospect

The fix was easy enough. We just needed to use some synchronization mechanism to ensure that the list doesn’t get modified by other thread when it was being iterated.

The interesting aspect was that it was not flagged earlier because of the combination of following things:

  1. IntRange getting finalized with start = 0 and last = observers.size before the loop runs.
  2. There were no deletions/removals happenings from the observers list so the other infamous NoSuchElementException exception also doesn’t occur.

In cases where I fix not so common bugs in my Kotlin code I try to verify the behaviour by looking at the decompiled bytecode. For this I use Android Studio’s inbuilt tool which can be invoked by Tools > Kotlin > Show Kotlin Bytecode. Then click on the Decompile button present top left of the opened window.

// plain kotlin code
for (observer in observers) {
observer.onStatusChange()
}
// decompiled bytecode of the above for loop
Iterator var2 = ((Scratch_2)this).observers.iterator();
while(var2.hasNext()) {
Observer observer = (Observer)var2.next();
observer.onStatusChange();
}

Decompiled Bytecode for ListIterator

 

For the above case of ListIterator decompiled bytecode everything seems ok as per our understanding that we got earlier.

// plain kotlin code
for (i in 0 until observers.size) {
observers[i].onStatusChange()
}
// decompiled bytecode of the above for loop
int i = 0;
for(int var5 = ((Scratch_2)this).observers.size(); i < var5; ++i) {
((Observer)((Scratch_2)this).observers.get(i)).onStatusChange();
}

Decompiled Bytecode for IntProgressionIterator

 

But for the case of IntProgressionIterator we can see that it is not being used at all and the for loop with IntRange got converted to plain for loop 😲. Nonetheless the behaviour that we might’ve gotten from using IntProgressionIterator doesn’t seems to have changed.

Although the fixes suggested by Android Studio doesn’t alter the intended behaviour of you code, it is always safer option to check the refactored code thoroughly.

Hope you liked reading the article. Reach out to me on Twitter. Thank You!

 

This article was originally published on proandroiddev.com on September 15, 2022

YOU MAY BE INTERESTED IN

YOU MAY BE INTERESTED IN

blog
It’s one of the common UX across apps to provide swipe to dismiss so…
READ MORE
blog
Hi, today I come to you with a quick tip on how to update…
READ MORE
blog
Automation is a key point of Software Testing once it make possible to reproduce…
READ MORE
blog
Drag and Drop reordering in Recyclerview can be achieved with ItemTouchHelper (checkout implementation reference).…
READ MORE

Leave a Reply

Your email address will not be published. Required fields are marked *

Fill out this field
Fill out this field
Please enter a valid email address.

Menu