spotify/async-datastore-client

Race condition when GoogleCredentials instance is shared by two threads

mk6i opened this issue · 0 comments

mk6i commented

When I pass an instance of com.google.api.client.auth.oauth2.Credential to the Datastore client that is shared with another library that also periodically refreshes the access token, the Datastore client occasionally fails with the following stack trace:

com.spotify.asyncdatastoreclient.DatastoreException: Request had invalid authentication credentials. Expected OAuth 2 access token, login cookie or other valid authentication credential. See https://developers.google.com/identity/sign-in/web/devconsole-project. 
at com.spotify.asyncdatastoreclient.DatastoreImpl.lambda$executeAsync$6(DatastoreImpl.java:280) at com.google.common.util.concurrent.AbstractTransformFuture$AsyncTransformFuture.doTransform(AbstractTransformFuture.java:221) at 
com.google.common.util.concurrent.AbstractTransformFuture$AsyncTransformFuture.doTransform(AbstractTransformFuture.java:210) at 
com.google.common.util.concurrent.AbstractTransformFuture.run(AbstractTransformFuture.java:130) at 
com.google.common.util.concurrent.MoreExecutors$DirectExecutor.execute(MoreExecutors.java:399) at 
com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:973) at 
com.google.common.util.concurrent.AbstractFuture.complete(AbstractFuture.java:821) at 
com.google.common.util.concurrent.AbstractFuture.access$500(AbstractFuture.java:68) at 
com.google.common.util.concurrent.AbstractFuture$SetFuture.run(AbstractFuture.java:299) at 
com.google.common.util.concurrent.MoreExecutors$DirectExecutor.execute(MoreExecutors.java:399) at 
com.ning.http.client.listenable.ExecutionList$RunnableExecutorPair.execute(ExecutionList.java:128) at 
com.ning.http.client.listenable.ExecutionList.run(ExecutionList.java:113) at 
com.ning.http.client.listenable.AbstractListenableFuture.runListeners(AbstractListenableFuture.java:67) at 
com.ning.http.client.providers.netty.future.NettyResponseFuture.done(NettyResponseFuture.java:226) at 
com.ning.http.client.providers.netty.handler.HttpProtocol.finishUpdate(HttpProtocol.java:194) at 
com.ning.http.client.providers.netty.handler.HttpProtocol.handleChunk(HttpProtocol.java:451) at 
com.ning.http.client.providers.netty.handler.HttpProtocol.handle(HttpProtocol.java:474) at 
com.ning.http.client.providers.netty.handler.Processor.messageReceived(Processor.java:88) at 
org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70) at 
org.jboss.netty.channel.DefaultChannelPipeline.sendUpstream(DefaultChannelPipeline.java:564) at 
org.jboss.netty.channel.DefaultChannelPipeline$DefaultChannelHandlerContext.sendUpstream(DefaultChannelPipeline.java:791) at 
org.jboss.netty.handler.stream.ChunkedWriteHandler.handleUpstream(ChunkedWriteHandler.java:142) at 
org.jboss.netty.channel.DefaultChannelPipeline.sendUpstream(DefaultChannelPipeline.java:564) at 
org.jboss.netty.channel.DefaultChannelPipeline$DefaultChannelHandlerContext.sendUpstream(DefaultChannelPipeline.java:791) at 
org.jboss.netty.handler.codec.http.HttpContentDecoder.messageReceived(HttpContentDecoder.java:132) at 
org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70) at 
org.jboss.netty.channel.DefaultChannelPipeline.sendUpstream(DefaultChannelPipeline.java:564) at org.jboss.netty.channel.DefaultChannelPipeline$DefaultChannelHandlerContext.sendUpstream(DefaultChannelPipeline.java:791) at org.jboss.netty.channel.Channels.fireMessageReceived(Channels.java:296) at org.jboss.netty.handler.codec.frame.FrameDecoder.unfoldAndFireMessageReceived(FrameDecoder.java:459) at org.jboss.netty.handler.codec.replay.ReplayingDecoder.callDecode(ReplayingDecoder.java:536) at org.jboss.netty.handler.codec.replay.ReplayingDecoder.messageReceived(ReplayingDecoder.java:435) at org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70) at org.jboss.netty.handler.codec.http.HttpClientCodec.handleUpstream(HttpClientCodec.java:92) at org.jboss.netty.channel.DefaultChannelPipeline.sendUpstream(DefaultChannelPipeline.java:564) at org.jboss.netty.channel.DefaultChannelPipeline$DefaultChannelHandlerContext.sendUpstream(DefaultChannelPipeline.java:791) at org.jboss.netty.channel.Channels.fireMessageReceived(Channels.java:296) at org.jboss.netty.handler.codec.frame.FrameDecoder.unfoldAndFireMessageReceived(FrameDecoder.java:462) at org.jboss.netty.handler.codec.frame.FrameDecoder.callDecode(FrameDecoder.java:443) at org.jboss.netty.handler.codec.frame.FrameDecoder.messageReceived(FrameDecoder.java:303) at org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70) at 
...

I narrowed down the cause to a race condition between the Datastore client and the other library that uses the credentials object. If the other library happens to call credential.refreshToken() before the Datastore client, the Datastore client will always see the Credentials token as "fresh". At some point the local accessToken copy will expire and stay expired as long as the other application refreshes the token before the Datastore client does, causing the stack trace above.

    // This code block is never true for as long as the other app preemptively calls `credential.refreshToken();` 
    if (this.accessToken == null
        || credential.getAccessToken() == null
        || expiresIn != null && expiresIn <= 60) {
      try {
        credential.refreshToken();
        final String accessTokenLocal = credential.getAccessToken();
        if (accessTokenLocal != null) {
          this.accessToken = accessTokenLocal;
        }
      } catch (final IOException e) {
        log.error("Storage exception", Throwables.getRootCause(e));
      }
    }

One workaround is to ensure that the Datastore client has exclusive access to the Credentials instance by creating separate Credentials instances per thread. Unfortunately, this is not possible when the credentials are loaded from ApplicationDefaultCredentials because GoogleCredential.getApplicationDefault().createScoped(DatastoreConfig.SCOPES) is effectively a singleton (See https://github.com/googleapis/google-auth-library-java/blob/13c2418a6c511765761de9e52a9f03016044dae8/oauth2_http/java/com/google/auth/oauth2/DefaultCredentialsProvider.java#L123).