[android] Produce a more visible error when not calling SDK methods from the UiThread

[Guardiola31337]

• @jfirebaugh noticed that there are a significant number of issues caused by not adhering to the @UiThread annotations and calling SDK methods from a background thread.

Because thread annotations are simply Lint checks and do not affect one’s build or compilation @ivovandongen suggested that we can do two things here, depending on how many methods it concerns:

• Create an annotation processor that inserts actual checks
• Add some manual checks on key places to verify which thread your code is running on

A compilation-time error would be ideal, but if that's not possible a runtime error would be ok.

cc/ @jfirebaugh @ivovandongen @zugaldia

[zugaldia]

cc: @tobrun

[LukasPaczos]

To add something to this one - there is no way to add code to an existing Java class via an annotation processor. There was a project called Lombok some time ago, but it looks like it's dead and here is the trick they've used.

Therefore, I think that only

Add some manual checks on key places to verify which thread your code is running on

is applicable here.

Another, more complicated approach, would be to wrap all public methods of a class that we want to "thread-check" with an interface and push through Proxy. For example, in OnMapReadyCallback instead of returning MapboxMap we would return an interface with all the public methods of MapboxMap and then with each method call we would be able to use predefined InvocationHandler:

  private static class MyInvocationHandler implements InvocationHandler {

    private final MapboxMap mapboxMap;

    public MyInvocationHandler(MapboxMap mapboxMap) {
      this.mapboxMap = mapboxMap;
    }

    @Override
    public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
      if (!Looper.myLooper().equals(Looper.getMainLooper())) {
        throw new IllegalThreadStateException();
      }

      return method.invoke(mapboxMap, args);
    }
  }

Above change is probably cleaner but it's a SEMVER change, adds another layer of complication and code would be a bit harder to maintain, so I don't think it's worth it here.

[tobrun]

Great analysis @LukasPaczos, one thing that came to mind for the manual approach is that we can guard a lot of the API through the code generation of the template files. This will cover a large part of the API though we still need to manually update components as MapboxMap, MapView, Projection etc.

[LukasPaczos]

Good point @tobrun, but basically everything that's generated is finally pushed through the pipeline of MapboxMap, MapView, Projection... and we don't want to restrict constructors, getters, setters etc. of Layers and Properties to UI Thread only, just the final call pushing it to the map.

It seems like all comes down to updating the methods ourselves.

[tobrun]

everything that's generated is finally pushed through the pipeline.. and we don't want to restrict constructors, getters, setters etc.

That is true for most cases but since Layer/Source are peer components, they can execute native code without involving the pipeline. Eg. GeoJSONSource#setGeoJson directly invokes the c++ equivalent hook without any other component involved. So yes, we need to restrict constructors, getters, .. when applicable.

[LukasPaczos]

Oh yes, thanks for emphasizing that @tobrun, I missed it. In this case, adding checks to the code templates will cover a decent chunk of the API.

[jfirebaugh]

#11978