Better error handling for OSM API errors

[strump]

Currently such methods as OsmOAuth.nativeGetOsmUsername() and OsmOAuth.nativeGetOsmChangesetsCount() doesn't provide detailed error message in case of troubles. If case of error first method returns null or -1.

API errors could be:

• Device is offline
• Host not found (DNS error)
• Network timeout
• HTTP errors 404, 401, 403, 500

Network errors could be ignored till next API try. But errors 401 and 403 should be handled in Java/Swift code to reset authentication and ask users to relogin.

Q: How to pass error type from native code to Java/Swift? Use magic constants as return types? Or return pair of result and error code?

[biodranik]

Define explicit enum values mapped to integers, that will be duplicated in Java and reused directly in ObjC. Choose only those errors that should be handled in the UI somehow, e.g.
EOk
ENetworkError
EAuthorizationFailed

[strump]

@biodranik the proble is that OsmOAuth.nativeGetOsmUsername() method returns String while OsmOAuth.nativeGetOsmChangesetsCount() returns Integer.

Having two enums is not handy:

• EIntOk, EStrOk
• EIntNetworkError, EStrNetworkError
• EIntAuthorizationFailed, EStrAuthorizationFailed

I don't want to use WINAPI style and introduce GetLastError method. But I think error must be passed to Java explicitly.

[biodranik]

There are many ways to approach this issue. For example https://stackoverflow.com/questions/29043872/android-jni-return-multiple-variables

A more robust and future-proof solution requires some refactoring. The main issue with the current approach is that the networking code in C++ auth is synchronous. And callers (java and ObjC) should do it in separate threads. And then these separate threads block. And only one value can be returned from synchronous calls.

A better approach would be to refactor the code (it can be done in parallel to existing code, without breaking it, and only remove existing code/tests when the new code is written) so it will accept a callback from the UI. And call it asynchronously with any number of parameters.

[strump]

Android documentation tells: Avoid asynchronous communication between code written in a managed programming language and code written in C++ when possible. Google proposes doing blocking JNI calls from dedicated Java thread.

Other option discussed in Google Groups "Intra-process queue between Java and C++".

I prefer blocking calls from Java thread to C++ code with Boost.Either as a return type. So we can pass error explicitly from C++ method and will need to convert it to some Java instance in JNI. With Swift we don't need to do convertion and can use Either object explicitly.

[biodranik]

@strump blocking threads is a wrong pattern for many reasons. Proper async communication should be done via callbacks, without blocking calls at all. Then there's no need to return multiple values and depend on some 3party libraries for that. And the whole app design/performance is cleaner.

On iOS, the system already works with async API. On Android, a bit of wrapping is needed.

This async HTTP API becomes more important in the context of the upcoming online features: satellite imagery, traffic info, public transport, photos and reviews, cloud sync, etc.

This requires balls though. Refactoring current Http client methods into something more robust, and async, is a good amount of work.

It could be safer to start building a better API in parallel to the existing one, and then migrate the existing code cases one by one.

[strump]

@biodranik iOS code is fine because it has no JNI. With Java everything becomes harder. We have Java → C++ → Java calls. We need to pass callback(s) through JNI in both directions Java ↔ C++.

For example:

1. UI thread invokes OSM API passing oauth2_token and Java callback(s)
2. JNI passes oauth2_token and a C++ callback(s) to class ServerApi06
3. C++ code invokes HttpClient passing URL, HTTP headers and a C++ callback(s)
4. HttpClient through JNI invokes Java class app.organicmaps.util.HttpClient passing URL, HTTP headers and a Java callback(s).
5. Java makes actual HTTP requests and invokes a Java callback.
6. Java callback then invokes C++ callback which invokes Java callback from step 1.

It feels like JNI addes too much overhead here already. Having additional callbacks in C++ code looks too complex. I would rather implement blocking C++ code and have callbacks only in Java code.

[biodranik]

What you described is already working. It doesn't feel great, but it works. The overhead can be minimized if threads are reused instead of being recreated each time.
There are reasons on why it was done like that.

The reality is that C++ needs to have a fully working HTTP client. It is guaranteed only by using iOS and Android classes that properly manage proxies, VPN, network switching, etc. So there's no choice to use some C++ http client.

Then iOS/Android code uses the same functionality under the hood (editor, traffic info, bookmarks sync, whatever), and duplicating it on each platform is not feasible.

Using blocked API has many limitations and complications, hard to maintain, doesn't follow system guidelines, and consumes a lot of resources in parallel execution (imagine creating thread for each request instead of using GCD or some thread pool).

Using asynchronous API with clearly defined callbacks allows to handle any type of issues efficiently.

[strump]

Callbacks are great, but it's not a silver bullet. We still need to manage threads and thread pools to separate UI and Callback calls. I propose to handle callback threads (thread pools) in Java/Swift code and keep C++ code as simple as possible.

[biodranik]

There are cases when C++-only threads should be used, without any Java/JNI overhead. It would be great to hide these details under the generic interface (it is already implemented in a similar way btw, did you check existing thread pools?).

When a task is launched on the thread pool, it can explicitly specify the desired thread (GUI) and desired priority. Any callback that may be run on any thread can add a task to the GUI thread to notify it, for example.