AppAuth-Android icon indicating copy to clipboard operation
AppAuth-Android copied to clipboard

performActionWithFreshTokens forces the callback to be ran in the UI/main thread

Open camhart opened this issue 9 years ago • 18 comments
trafficstars

AuthState.performActionWithFreshTokens runs the AuthState.Action callback on the same thread as the calling thread when the token doesn't need refreshed, but when it does need refreshed it's always ran on the main/UI thread. I think this needs to get changed so that the AuthState.Action callback is always ran on either a background thread or the main/UI thread regardless of whether the token gets refreshed or not. And whichever is chosen should be clearly documented for other developers to read.

My guess is the programmers of AuthState.performActionWithFreshTokens assumed it would always be called from the main/UI thread... but there are situations where that's not the case.

camhart avatar Nov 06 '16 23:11 camhart

We hadn't documented the threading behavior of our callbacks and this is clearly an oversight, thanks for bringing it to our attention. I don't know if the callback should necessarily be forced to execute on the main thread or a background thread; this should likely be up to the developer. Perhaps this should be a parameter on the API itself, such as providing an Executor to indicate where the task should be run. I'll give it some thought for 0.5.0.

iainmcgin avatar Nov 08 '16 19:11 iainmcgin

Another option would be to just leave it up to the developer to ensure the AuthState.performActionWithFreshTokens call is done within a background thread (and then be sure to update documentation to point out that thats needed). This leaves full control in the developers hands from what I can tell.

Plus in case the developer fails to read the documentation they'll get a nice exception telling them to change it.

camhart avatar Nov 09 '16 07:11 camhart

We should sync? https://github.com/openid/AppAuth-iOS/issues/46

StevenEWright avatar Dec 06 '16 17:12 StevenEWright

I vote for a thread stable approach. If the call was made from the UI thread then callback on the UI thread. If the call was made from background thread callback on background thread (never UI) regardless of the state of the refresh. It is the difference in thread treatment depending on if the refresh is performed or not that I tripped over.

darrenjoconnor avatar Dec 09 '16 23:12 darrenjoconnor

As a temporary fix: https://github.com/camhart/AppAuth-Android

camhart avatar Mar 10 '17 02:03 camhart

Any new thoughts on this? My vote is also on the thread stable approach.

raehalme avatar Jul 05 '17 06:07 raehalme

My intention is to provide two variants of the method, to which you can supply an Executor or a Handler. This will allow you to select the thread used with precision. The default behavior (where no executor or handler are provided) will remain the same.

iainmcgin avatar Jul 05 '17 15:07 iainmcgin

I also am very interested in a fix for this issue. Control over which thread/executor runs the background action and, more importantly, which runs the callback is key to some use cases. Insight into the timing of the 0.8.0 release would thus be appreciated.

JessHolle avatar Aug 31 '17 12:08 JessHolle

We generally don't commit to timelines for the releases, as we don't have any full-time maintainers (and my own time is stretched very thin across multiple efforts). However, I'll see what I can do about getting a fix for this in this week.

iainmcgin avatar Oct 03 '17 22:10 iainmcgin

I ran into this problem, in my app I'm doing work in a background thread already.

I make a call to performActionWithFreshTokens and in the callback, with the Access Token, I wanted to make an API call, resulting in a NetworkOnMainThreadException.

The workaround I've done is to start a HandlerThread just before calling the fresh tokens method, then in the callback, using Handler.post to continue execution.

    handlerThread = new HandlerThread("HandlerThread");
    handlerThread.start();
    handler = new Handler(handlerThread.getLooper());

    authState.performActionWithFreshTokens(authorizationService, new AuthState.AuthStateAction() {
                @Override
                public void execute(@Nullable String accessToken, @Nullable String idToken, @Nullable AuthorizationException ex) {
                    if (ex != null) {
                        //error handling here
                        return;
                    }
                    handler.post(new Runnable() {  ... });  // actual network call here. 
                }
            });

The sad news - this approach won't work if you need to wait for the response from your API call. Trying to work around it by waiting for a handler/notify instead causes the entire UI to lock up. :disappointed:

mendhak avatar May 28 '22 18:05 mendhak

We surprisingly realized in the last days, that the callback is running in main-thread which is causing big problems in some situations.

Any updates on this?

MasterEmit avatar Nov 08 '22 12:11 MasterEmit

@MasterEmit this project is abandoned. I wouldnt recommend you use it.

I'm not the maintainer, but no issues have been fixed for years.

camhart avatar Nov 08 '22 16:11 camhart

I don't see another general purpose oauth/oidc library that's as broadly recommended for Android. Is there one? Especially one that's got a very similar sibling on iOS as well?

There have been changes to this library in the last year, but, yes, they were minimal to account for new Android OS versions, etc.

JessHolle avatar Nov 08 '22 17:11 JessHolle

No one recommends this from what I've seen, aside from Google but its a google engineer who abandoned it and no other engineers picked it up.

You're welcome to use it. Just sharing my experience.

I'd use Aws cognito, or Auth0, etc.

camhart avatar Nov 08 '22 17:11 camhart

Aws cognito or Auth0 are bound to their services. What we need is a general purpose oauth/oidc library. We also searched if we can find something else and sadly nothing available for Android. For all other platforms its easy. I can't believe, that this is the reality in 2022. Of course we could write the whole process by ourself, but this makes no sense for such a standardized way of authentication.

There are other companies who are offering authentication services and they also recommend this library for Android. Really strange.

MasterEmit avatar Nov 08 '22 17:11 MasterEmit

For the record, we are successfully using this library. And, yes, we're using it because:

  1. We're using AppAuth on iOS.
  2. This library is not bound to or biased towards any specific service.

As for why we're not encountering a problem with the issue noted here, I suspect (but haven't verified that) it's because some other worker thread is always calling an action to obtain a fresh access token, e.g. as a CompletableFuture, and then continuing back on a worker thread. Yes, if the callback is forced onto the UI thread that's still not ideal -- as it causes unnecessary delay. But it would explain why we're not seeing issues.

JessHolle avatar Nov 08 '22 17:11 JessHolle

I was using it successfully as well. But I switched due to a number of bugs, this being one of them. I also found a way to handle this issue, but it is a hack to have to do so. I wouldn't choose to use this library.

camhart avatar Nov 08 '22 17:11 camhart

@camhart -- What did you switch to? Is there a better maintained OIDC library for kotlin + Android?

atrauzzi avatar Jun 30 '23 13:06 atrauzzi