Fix ConconcurrentModificationException in Maps

[jonathanklee]

Taxi G7 app was throwing this exception at startup.

It seems like Taxi G7 callbacks are adding new callbacks to execute.

[fynngodau]

That the app was previously able to add callbacks was probably also a bug that is related to synchronization code. Without it fixed, as-is, (1) the callback would get executed from the list, (2) the app adds new callbacks during (1), and (3) afterwards the callbacks are cleared (L 874).

Do you know what callbacks the app added / what methods it called?

[jonathanklee]

@fynngodau Yes I agree. The whole workflow seems a bit precarious as is. It seems like a callback is not being executed. I'll have a look at it.

[fynngodau]

Generally speaking, I'm not even sure what the reason for the mapLock variable is. It could be replaced by this as the object that is synchronized at as far as I can tell.

[jonathanklee]

@fynngodau I guess it's to have more granularity on the lock time since there are already synchronized(this) instructions in the class.

[mar-v-in]

Shouldn't this still all happen within one synchronized block? Otherwise, if getMapAsync is invoked twice at the same time, it may interleave in such a way that only one callback is stored.

Also, what would probably be much easier, would be to use the java.util.concurrent.CopyOnWriteArrayList - it handles this internally for you.

I do see the issue though, that if getMapAsync is invoked from within the map ready callback, that getMapAsync will never get it's callback invoked (as the clearing of the callback list is not tied to the actual list of callbacks that was invoked - that's not a new issue though). The better approach thus could be to change the calling code to create a copy of the list before invoking the callbacks, then clear the original list and then start invoking the callbacks from the copy. As the list that is iterated is a local copy, no ConcurrentModificationException could happen either and we wouldn't see the issue of callbacks getting never called.

[mar-v-in]

Generally speaking, I'm not even sure what the reason for the mapLock variable is. It could be replaced by this as the object that is synchronized at as far as I can tell.

The motivation of using an explicit private lock is that it makes it impossible for code outside the class to acquire the lock. If you use this as a lock, everyone with a reference to the object can acquire the lock which may lead to unexpected and unwanted side effects.

That said, the usage of locks in this class specifically is probably not very consistent.