Fix pendingWrite race condition

pendingWrites.add() was not guaranteed to be called before pendingWrites.remove()
googleapis · Mar 19, 2019 · a083e13 · a083e13
1 parent 0a6fc39
commit a083e13
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 3 deletions.
diff --git a/...loud-clients/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java b/...loud-clients/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
@@ -37,6 +37,7 @@
 import com.google.cloud.MonitoredResourceDescriptor;
 import com.google.cloud.PageImpl;
 import com.google.cloud.logging.spi.v2.LoggingRpc;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
@@ -595,6 +596,9 @@ private void writeLogEntries(Iterable<LogEntry> logEntries, WriteOption... write
       case ASYNC:
       default:
         final ApiFuture<Void> writeFuture = writeAsync(logEntries, writeOptions);
+        synchronized (writeLock) {
+          pendingWrites.add(writeFuture);
+        }
         ApiFutures.addCallback(
             writeFuture,
             new ApiFutureCallback<Void>() {
@@ -619,9 +623,6 @@ public void onFailure(Throwable t) {
                 }
               }
             });
-        synchronized (writeLock) {
-          pendingWrites.add(writeFuture);
-        }
         break;
     }
   }
@@ -707,4 +708,11 @@ public void close() throws Exception {
     }
     return optionMap;
   }
+
+  @VisibleForTesting
+  int getNumPendingWrites() {
+    synchronized (writeLock) {
+      return pendingWrites.size();
+    }
+  }
 }
diff --git a/...-clients/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java b/...-clients/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
@@ -1186,6 +1186,7 @@ public void testWriteLogEntries() {
     EasyMock.replay(rpcFactoryMock, loggingRpcMock);
     logging = options.getService();
     logging.write(ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2));
+    assertEquals(0, ((LoggingImpl) logging).getNumPendingWrites());
   }
 
   @Test