pubsub: fix race condition in streaming connection

google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/StreamingSubscriberConnection.java

@@ -38,6 +38,8 @@
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.Nullable;
@@ -52,14 +54,15 @@ final class StreamingSubscriberConnection extends AbstractApiService implements
   private static final Duration MAX_CHANNEL_RECONNECT_BACKOFF = Duration.ofSeconds(10);
   private static final int MAX_PER_REQUEST_CHANGES = 10000;
 
-  private final AtomicLong channelReconnectBackoffMillis =
-      new AtomicLong(INITIAL_CHANNEL_RECONNECT_BACKOFF.toMillis());
-
   private final SubscriberStub asyncStub;
-
   private final String subscription;
   private final ScheduledExecutorService executor;
   private final MessageDispatcher messageDispatcher;
+
+  private final AtomicLong channelReconnectBackoffMillis =
+      new AtomicLong(INITIAL_CHANNEL_RECONNECT_BACKOFF.toMillis());
+
+  private final Lock lock = new ReentrantLock();
   private ClientCallStreamObserver<StreamingPullRequest> requestObserver;
 
   public StreamingSubscriberConnection(
@@ -101,23 +104,35 @@ protected void doStart() {
   @Override
   protected void doStop() {
     messageDispatcher.stop();
-    requestObserver.onError(Status.CANCELLED.asException());
-    notifyStopped();
+
+    lock.lock();
+    try {
+      requestObserver.onError(Status.CANCELLED.asException());
+    } finally {
+      lock.unlock();
+      notifyStopped();
+    }
   }
 
   private class StreamingPullResponseObserver
       implements ClientResponseObserver<StreamingPullRequest, StreamingPullResponse> {
 
     final SettableFuture<Void> errorFuture;
+    ClientCallStreamObserver<StreamingPullRequest> thisRequestObserver;
 
     StreamingPullResponseObserver(SettableFuture<Void> errorFuture) {
       this.errorFuture = errorFuture;
     }
 
     @Override
     public void beforeStart(ClientCallStreamObserver<StreamingPullRequest> requestObserver) {
-      StreamingSubscriberConnection.this.requestObserver = requestObserver;
-      requestObserver.disableAutoInboundFlowControl();
+      thisRequestObserver = requestObserver;
+      lock.lock();
+      try {
+        requestObserver.disableAutoInboundFlowControl();
+      } finally {
+        lock.unlock();
+      }
     }
 
     @Override
@@ -128,9 +143,22 @@ public void onNext(StreamingPullResponse response) {
           new Runnable() {
             @Override
             public void run() {
-              // Only if not shutdown we will request one more batches of messages to be delivered.
+              /**
+               * Only if not shutdown we will request one more batches of messages to be delivered.
+               *
+               * <p>We use the request observer we're paired with, not the "current observer" in the
+               * outer class. By the time this Runnable is called, the stream might have already
+               * failed and the outer class might have created a new stream. In this case, we should
+               * request more messages from the already-failed stream (a no-op), otherwise the
+               * current observer might get more messages than it should.
+               */
               if (isAlive()) {
-                requestObserver.request(1);
+                lock.lock();
+                try {
+                  thisRequestObserver.request(1);
+                } finally {
+                  lock.unlock();
+                }
               }
             }
           });
@@ -166,6 +194,18 @@ private void initialize() {
             .build());
     requestObserver.request(1);
 
+    /**
+     * Must make sure we do this after sending the subscription name and deadline. Otherwise, some
+     * other thread might use this stream to do something else before we could send the first
+     * request.
+     */
+    lock.lock();
+    try {
+      this.requestObserver = requestObserver;
+    } finally {
+      lock.unlock();
+    }
+
     Futures.addCallback(
         errorFuture,
         new FutureCallback<Void>() {
@@ -188,24 +228,24 @@ public void onFailure(Throwable cause) {
               return;
             }
             logger.log(Level.WARNING, "Terminated streaming with exception", cause);
-            if (StatusUtil.isRetryable(cause)) {
-              long backoffMillis = channelReconnectBackoffMillis.get();
-              long newBackoffMillis =
-                  Math.min(backoffMillis * 2, MAX_CHANNEL_RECONNECT_BACKOFF.toMillis());
-              channelReconnectBackoffMillis.set(newBackoffMillis);
-
-              executor.schedule(
-                  new Runnable() {
-                    @Override
-                    public void run() {
-                      initialize();
-                    }
-                  },
-                  backoffMillis,
-                  TimeUnit.MILLISECONDS);
-            } else {
+            if (!StatusUtil.isRetryable(cause)) {
               notifyFailed(cause);
+              return;
             }
+            long backoffMillis = channelReconnectBackoffMillis.get();
+            long newBackoffMillis =
+                Math.min(backoffMillis * 2, MAX_CHANNEL_RECONNECT_BACKOFF.toMillis());
+            channelReconnectBackoffMillis.set(newBackoffMillis);
+
+            executor.schedule(
+                new Runnable() {
+                  @Override
+                  public void run() {
+                    initialize();
+                  }
+                },
+                backoffMillis,
+                TimeUnit.MILLISECONDS);
           }
         },
         executor);
@@ -220,8 +260,13 @@ public void sendAckOperations(
       List<String> acksToSend, List<PendingModifyAckDeadline> ackDeadlineExtensions) {
     List<StreamingPullRequest> requests =
         partitionAckOperations(acksToSend, ackDeadlineExtensions, MAX_PER_REQUEST_CHANGES);
-    for (StreamingPullRequest request : requests) {
-      requestObserver.onNext(request);
+    lock.lock();
+    try {
+      for (StreamingPullRequest request : requests) {
+        requestObserver.onNext(request);
+      }
+    } finally {
+      lock.unlock();
     }
   }
 
@@ -271,9 +316,14 @@ static List<StreamingPullRequest> partitionAckOperations(
 
   public void updateStreamAckDeadline(int newAckDeadlineSeconds) {
     messageDispatcher.setMessageDeadlineSeconds(newAckDeadlineSeconds);
-    requestObserver.onNext(
-        StreamingPullRequest.newBuilder()
-            .setStreamAckDeadlineSeconds(newAckDeadlineSeconds)
-            .build());
+    lock.lock();
+    try {
+      requestObserver.onNext(
+          StreamingPullRequest.newBuilder()
+              .setStreamAckDeadlineSeconds(newAckDeadlineSeconds)
+              .build());
+    } finally {
+      lock.unlock();
+    }
   }
 }