make requests wait for api to initialize properly

[David-Development]

As mentioned here, when making requests right after starting the NextcloudAPI, the request will fail with the following error message:

Attempt to invoke interface method 'android.os.ParcelFileDescriptor com.nextcloud.android.sso.aidl.IInputStreamService.performNextcloudRequest(android.os.ParcelFileDescriptor)' on a null object reference

This happens because the service, which is used for passing the actual network request to the files app is not started/bound at that very moment. Therefore we need to wait for the service to be bound.

This PR adds a check before each call waiting for the API to be initialized. If the API is not ready within 10 seconds, the request will fail and a NextcloudApiNotRespondingException will be thrown.

Let me know what you think!

[desperateCoder]

If anyone is interested, this is how we solved this in the Deck-App:
https://github.com/stefan-niedermann/nextcloud-deck/blob/master/app/src/main/java/it/niedermann/nextcloud/deck/api/RequestHelper.java

Looks confusing, but the call looks like this in the end:

RequestHelper.request(sourceActivity, provider, () -> provider.getAPI().getStack(boardId, stackId, getLastSync()), responseCallback);

For further information, just ask. :)

[David-Development]

@desperateCoder Oh sweet, I didn't know you guys already had a workaround for that issue!

If we merge the changes in this repo, would that make your code obsolete? A cheeky question: Is there any advantage of the callbacks and async stuff over this solution which leaves the waiting for the API tasks to the sso library. Otherwise all third party app developers have to handle this themselves..

The user/developer shouldn't have to care about waiting for the api to be ready. This should be handled internally. So you as a developer can just make the call without needing to worry about it. I think this would make the whole onConnected callback somewhat obsolete. Just init the API, check for errors and make your requests. I already updated the docs accordingly: https://github.com/nextcloud/Android-SingleSignOn/blob/master/README.md (example in section 4.2)

And you wouldn't have to keep track of whether the API is connected or not. (https://github.com/stefan-niedermann/nextcloud-deck/blob/master/app/src/main/java/it/niedermann/nextcloud/deck/api/ApiProvider.java#L34)

Let me know what you think! :)

[desperateCoder]

Hi @David-Development,
Yes, the code would most likely get obsolete, but i'm totally fine with that.
Just one thing: please don't block the thread while intitalizing the API. Maybe you can even integrate the RequestHelper into your code. I would like to avoid being blocked by the init.

But since we are already talking about the current drawbacks: Something that makes life really hard is, that for the SSO-Calls, Retrofit gets kind of useless.
The API takes NextcloudRequests, but the standard Retrofit-Libs work with Calls and even Observables, containing all the data a NextcloudRequest needs. Is there any chance, you could allow Observables instead or additionally?

[David-Development]

please don't block the thread while intitalizing the API

Can you elaborate a little more on this please? I was thinking about this as well. You solved this in the RequestHelper by using callbacks and sync methods.

The wait for api check happens right before the network request will be executed. From my understanding this should be blocking. Otherwise you'll have another layer of indirection here.. The thread is basically just waiting a couple of milliseconds before actually making the request which might take a couple of seconds anyways.

This is only a problem if you use the main-thread to make a network request as this will block the UI as well as the API connect. That's why I added the NetworkOnMainThreadException so that you have to use a different Thread than the Main-Thread. Using the main thread doesn't work anyways as the files app wouldn't allow making the request on the main thread.

The API takes NextcloudRequest, but the standard Retrofit-Libs work with Calls and even Observables, containing all the data a NextcloudRequest needs. Is there any chance, you could allow Observables instead or additionally?

Can you elaborate a little more on this please? Do you have some examples for me? (Code references?)

Are you referring to something like this?

// Interface: 
@POST("feeds")
Call<List<Feed>> createFeed(@Body Map<String, Object> feedMap);

// NextcloudAPI
@Override
public Call<List<Feed>> createFeed(Map<String, Object> feedMap) {
    Type feedListType = new TypeToken<List<Feed>>() {}.getType();
    String body = GsonConfig.GetGson().toJson(feedMap);
    NextcloudRequest request = new NextcloudRequest.Builder()
            .setMethod("POST")
            .setUrl(mApiEndpoint + "feeds")
            .setRequestBody(body)
            .build();
    return Retrofit2Helper.WrapInCall(nextcloudAPI, request, feedListType);
}

[desperateCoder]

Can you elaborate a little more on this please? I was thinking about this as well. You solved this in the RequestHelper by using callbacks and sync methods.

In short, what i mean is (pseudo code):

new Thread(() -> initSSO()).start();
new Thread(() -> sso.callSomeEndpoint()).start();

Would this work? Since it is possible, that the endpoint-call-thread is executed first (noone knows for sure), i am afraid of getting mysterious errors occassionally, something like "dumb desperateCoder, first initialize the api" while it looks like I already did. if this is guaranteed to work, never mind.

Are you referring to something like this?

Exactly. I have Retrofit, who is perfectly able to build my reqests without a single line of implementation, but I can't use it, since it does not know how to build a NextcloudRequest. I even tried to convert the Observable or Call to NextcloudRequest, but i had to give up because of architectual reasons.

[David-Development]

You should run it like this:

// Main Thread
SingleSignOnAccount ssoAccount = SingleAccountHelper.getCurrentSingleSignOnAccount(context);
NextcloudAPI nextcloudAPI = new NextcloudAPI(context, ssoAccount, GsonConfig.GetGson(), 
             new NextcloudAPI.ApiConnectedListener() {
                @Override public void onConnected() { }
                @Override
                public void onError(Exception ex) {
                    callback.onError(ex);
                }
            });
DeckAPI_SSO mApi = new DeckAPI_SSO(nextcloudAPI);

// Start API call in background thread
new Thread(() -> mApi.callSomeEndpoint()).start();

Or why would you init the api on a background thread?

In order to make a call to mApi.callSomeEndpoint() you need an instance of mApi (DeckAPI_SSO). If you run the initialization on a background thread, you need to synchronize this vairable, right? Therefore just run the instance creation of new NextcloudAPI(...) and new DeckAPI_SSO(...) on the main thread and pass the reference into the other thread. Hope that helps?

[desperateCoder]

Or why would you init the API on a background thread?

I'd like to avoid blocking the main thread (UI) from being blocked by this task. Thats my whole point. I can't tell what happens in the constructor of NextcloudAPI, but if you say that it is uncritical and returns fast, i'd be ok with that and your suggestion is totally fine.
One more concern: the ApiConnectedListener isn't invoked on the main Thread, right?

[David-Development]

@desperateCoder

I'd like to avoid blocking the main thread (UI) from being blocked by this task. Thats my whole point. I can't tell what happens in the constructor of NextcloudAPI, but if you say that it is uncritical and returns fast, i'd be ok with that and your suggestion is totally fine.

I understand your concern here. However the constructor of the NextcloudAPI returns immediately after calling. (https://github.com/nextcloud/Android-SingleSignOn/blob/master/src/main/java/com/nextcloud/android/sso/api/NextcloudAPI.java#L67)

Basically what happens is, it sets a couple of variables and then calls the method connectApiWithBackoff(). The service binding happens on the main thread as well as the callback. If you create the NextcloudAPI in a background thread, only the variable initialization will happen on the background thread. The call to connect() will automatically use the main thread. Take a look at line 95 for more information (Looper.getMainLooper()): https://github.com/nextcloud/Android-SingleSignOn/blob/master/src/main/java/com/nextcloud/android/sso/api/NextcloudAPI.java#L95

One more concern: the ApiConnectedListener isn't invoked on the main Thread, right?

It is always invoked on the main thread. Maybe we should force users to create the NextcloudAPI on the main thread.. This might prevent confusion about it?