openid / AppAuth-Android

Android client SDK for communicating with OAuth 2.0 and OpenID Connect providers.
https://openid.github.io/AppAuth-Android
Apache License 2.0
2.86k stars 887 forks source link

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

Open camhart opened 8 years ago

camhart commented 8 years ago

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.

iainmcgin commented 8 years ago

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.

camhart commented 8 years ago

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.

StevenEWright commented 7 years ago

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

darrenjoconnor commented 7 years ago

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.

camhart commented 7 years ago

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

raehalme commented 7 years ago

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

iainmcgin commented 7 years ago

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.

JessHolle commented 7 years ago

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.

iainmcgin commented 7 years ago

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.

mendhak commented 2 years ago

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:

MasterEmit commented 2 years ago

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?

camhart commented 2 years ago

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

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

JessHolle commented 2 years ago

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.

camhart commented 2 years ago

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.

MasterEmit commented 2 years ago

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.

JessHolle commented 2 years ago

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.

camhart commented 2 years ago

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.

atrauzzi commented 1 year ago

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