tangram-es icon indicating copy to clipboard operation
tangram-es copied to clipboard

MapController and MapData should have a way to query whether they have been disposed.

Open matteblair opened this issue 3 years ago • 2 comments

See related issue: https://github.com/streetcomplete/StreetComplete/issues/2879

Depending on an app's architecture, it might not be practical to release all references to MapController and MapData objects when the MapView is destroyed. For instance, an app might update MapData contents from a background thread.

To safeguard against accessing a disposed object in those cases, we can add a boolean property to these classes that would let the app choose how to proceed if the object it references is disposed.

matteblair avatar May 17 '21 18:05 matteblair

I also have another idea. Just to recap:

  1. it is good that tangram-es crashes when a MapController is accessed when its MapView has already been destroyed because it is easy to find the (memory leak) issue then
  2. thus, a "safe call" should be done by the user explicitly

How can such a safe call look? Currently, it would be

try {
  mapController?.doSomething()
} catch(e: Exception) {}

If the suggestion in this ticket is implemented, it would look like

if (mapController?.isAlive == true) {
  mapController?.doSomething()
}

There is a general issue though with this. Imagine the above code is executed not on the main thread. Then, it is at least theoretically possible that in between the first and the second call, the map view is destroyed.


So, here is another suggestion:

I understand that MapController is a field of MapView and when MapView is destroyed as part of the Fragments or Activitys lifecycle, supposedly the only reference to MapController is removed. If that is the case, then simply calling

  mapController?.doSomething()

is already safe. In other words, if MapController is wrapped in a WeakReference by the user, it's safe. Is this assumption correct? Then, the Android example(s) could just be adapted and done.

westnordost avatar May 17 '21 18:05 westnordost

No experience with Tangram, but it's general Java wisdom that the memory model is very clear in that, without synchronized, the JVM is allowed to transform the code, as long as it produces the same observable results as when it's run on a single thread. This includes inlining (if (... != null) inlined as true), instruction reordering, threads not seeing updates from each other (not syncing their local cached copies of the heap).

So the general wisdom is that all access (no exceptions) to shared resources must always go inside synchronized and that the only correct pattern is

(thread 1)
synchronized(lock) {
    a = null;
}

(thread 2)
synchronized(lock) {
    if (a != null)
        a.something();
}

where lock (as advised in "Effective Java") is typically private Object lock = new Object(); instead of this, just to make sure a client can't synchronize on the same object, messing with the lock.

For more discussion about what can go wrong, see the "memory model" link above and the famous book "Effective Java" by Joshua Bloch (I think that synchronization is Item 66) which is a must-read both for Java and Kotlin.

Does Kotlin a?.something() guarantee that it includes the synchronized?

opk12 avatar Sep 15 '23 17:09 opk12