Skip to content

Commit 9a70805

Browse files
michaeledgarcopybara-github
michaeledgar
authored and
copybara-github
committed
Fix double shutdown of BuildEventArtifactUploader when BES+File output enabled.
When both BES uploading and File BEP output are enabled, a single BuildEventArtifactUploader object is shared by two different BuildEventTransports. Both were calling #shutdown() which in turn called ByteStreamUploader#shutdown(). If shutdown is called while one transport is still uploading, the ByteStreamUploader will fail an assertion and crash. This change adds reference counting to the BuildEventArtifactUploader interface and ensures the reference counts are maintained correctly when sharing a BuildEventArtifactUploader across multiple independent BuildEventTransport threads. Fixes #12575. RELNOTES: None. TESTED=Made repro modifications to GrpcCacheClient.java described in #12575 and confirmed crash without this change. Implemented this fix, observed crash was no longer reproducible. Added logs to ByteStreamBuildEventArtifactUploader#deallocate() to verify deallocation happened after both BuildEventTransports had completed. PiperOrigin-RevId: 349589743
1 parent c833660 commit 9a70805

13 files changed

+90
-46
lines changed

‎src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -853,7 +853,8 @@ private static class ThrowingBuildEventArtifactUploaderSupplier {
853853
}
854854

855855
BuildEventArtifactUploader get() throws IOException {
856-
if (memoizedValue == null && exception == null) {
856+
boolean needsInitialization = memoizedValue == null;
857+
if (needsInitialization && exception == null) {
857858
try {
858859
memoizedValue = callable.call();
859860
} catch (IOException e) {
@@ -864,6 +865,9 @@ BuildEventArtifactUploader get() throws IOException {
864865
}
865866
}
866867
if (memoizedValue != null) {
868+
if (!needsInitialization) {
869+
memoizedValue.retain();
870+
}
867871
return memoizedValue;
868872
}
869873
throw exception;

‎src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceUploader.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,7 @@ public void run() {
355355
logger.atSevere().log("BES upload failed due to a RuntimeException / Error. This is a bug.");
356356
throw e;
357357
} finally {
358-
buildEventUploader.shutdown();
358+
buildEventUploader.release();
359359
MoreExecutors.shutdownAndAwaitTermination(timeoutExecutor, 0, TimeUnit.MILLISECONDS);
360360
closeFuture.set(null);
361361
}

‎src/main/java/com/google/devtools/build/lib/buildeventstream/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ java_library(
2929
"//third_party:flogger",
3030
"//third_party:guava",
3131
"//third_party:jsr305",
32+
"//third_party:netty",
3233
"//third_party/protobuf:protobuf_java",
3334
],
3435
)

‎src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploader.java

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile;
2121
import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType;
2222
import com.google.devtools.build.lib.vfs.Path;
23+
import io.netty.util.ReferenceCounted;
2324
import java.io.IOException;
2425
import java.io.InputStream;
2526
import java.io.OutputStream;
@@ -30,7 +31,7 @@
3031
import javax.annotation.Nullable;
3132

3233
/** Uploads artifacts referenced by the Build Event Protocol (BEP). */
33-
public interface BuildEventArtifactUploader {
34+
public interface BuildEventArtifactUploader extends ReferenceCounted {
3435
/**
3536
* Asynchronously uploads a set of files referenced by the protobuf representation of a {@link
3637
* BuildEvent}. This method is expected to return quickly.
@@ -78,11 +79,6 @@ public ListenableFuture<String> uriFuture() {
7879
}
7980
};
8081

81-
/**
82-
* Shutdown any resources associated with the uploader.
83-
*/
84-
void shutdown();
85-
8682
/**
8783
* Return true if the upload may be "slow". Examples of slowness include writes to remote storage.
8884
*/

‎src/main/java/com/google/devtools/build/lib/buildeventstream/LocalFilesArtifactUploader.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,16 @@
1818
import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile;
1919
import com.google.devtools.build.lib.buildeventstream.PathConverter.FileUriPathConverter;
2020
import com.google.devtools.build.lib.vfs.Path;
21+
import io.netty.util.AbstractReferenceCounted;
22+
import io.netty.util.ReferenceCounted;
2123
import java.util.Map;
2224
import java.util.Set;
2325
import java.util.concurrent.ConcurrentHashMap;
2426
import javax.annotation.Nullable;
2527

2628
/** An uploader that simply turns paths into local file URIs. */
27-
public class LocalFilesArtifactUploader implements BuildEventArtifactUploader {
29+
public class LocalFilesArtifactUploader extends AbstractReferenceCounted
30+
implements BuildEventArtifactUploader {
2831
private static final FileUriPathConverter FILE_URI_PATH_CONVERTER = new FileUriPathConverter();
2932
private final ConcurrentHashMap<Path, Boolean> fileIsDirectory = new ConcurrentHashMap<>();
3033

@@ -34,10 +37,15 @@ public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
3437
}
3538

3639
@Override
37-
public void shutdown() {
40+
protected void deallocate() {
3841
// Intentionally left empty
3942
}
4043

44+
@Override
45+
public ReferenceCounted touch(Object o) {
46+
return this;
47+
}
48+
4149
@Override
4250
public boolean mayBeSlow() {
4351
return false;

‎src/main/java/com/google/devtools/build/lib/buildeventstream/transports/FileTransport.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ public void run() {
156156
} catch (IOException e) {
157157
logger.atSevere().withCause(e).log("Failed to close BEP file output stream.");
158158
} finally {
159-
uploader.shutdown();
159+
uploader.release();
160160
timeoutExecutor.shutdown();
161161
}
162162
closeFuture.set(null);

‎src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@
3232
import com.google.devtools.build.lib.remote.util.DigestUtil;
3333
import com.google.devtools.build.lib.vfs.Path;
3434
import io.grpc.Context;
35+
import io.netty.util.AbstractReferenceCounted;
36+
import io.netty.util.ReferenceCounted;
3537
import java.io.IOException;
3638
import java.util.ArrayList;
3739
import java.util.HashMap;
@@ -43,10 +45,9 @@
4345
import java.util.concurrent.atomic.AtomicBoolean;
4446
import javax.annotation.Nullable;
4547

46-
/**
47-
* A {@link BuildEventArtifactUploader} backed by {@link ByteStreamUploader}.
48-
*/
49-
class ByteStreamBuildEventArtifactUploader implements BuildEventArtifactUploader {
48+
/** A {@link BuildEventArtifactUploader} backed by {@link ByteStreamUploader}. */
49+
class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
50+
implements BuildEventArtifactUploader {
5051

5152
private final ListeningExecutorService uploadExecutor;
5253
private final Context ctx;
@@ -243,14 +244,19 @@ public boolean mayBeSlow() {
243244
}
244245

245246
@Override
246-
public void shutdown() {
247+
protected void deallocate() {
247248
if (shutdown.getAndSet(true)) {
248249
return;
249250
}
250251
uploader.release();
251252
uploadExecutor.shutdown();
252253
}
253254

255+
@Override
256+
public ReferenceCounted touch(Object o) {
257+
return this;
258+
}
259+
254260
private static class PathConverterImpl implements PathConverter {
255261

256262
private final String remoteServerInstanceName;

‎src/main/java/com/google/devtools/build/lib/runtime/BuildEventArtifactUploaderFactory.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public interface BuildEventArtifactUploaderFactory {
2626

2727
/**
2828
* Returns a new instance of a {@link BuildEventArtifactUploader}. The call is responsible for
29-
* calling {@link BuildEventArtifactUploader#shutdown()} on the returned instance.
29+
* calling {@link BuildEventArtifactUploader#release()} on the returned instance.
3030
*/
3131
BuildEventArtifactUploader create(CommandEnvironment env)
3232
throws InvalidPackagePathSymlinkException;

‎src/test/java/com/google/devtools/build/lib/buildeventstream/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ java_test(
3030
"//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
3131
"//third_party:guava",
3232
"//third_party:junit4",
33+
"//third_party:netty",
3334
"//third_party:truth",
3435
],
3536
)

‎src/test/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderFactoryMapTest.java

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.google.devtools.build.lib.runtime.BuildEventArtifactUploaderFactoryMap;
2323
import com.google.devtools.build.lib.runtime.CommandEnvironment;
2424
import com.google.devtools.build.lib.vfs.Path;
25+
import io.netty.util.ReferenceCounted;
2526
import java.io.IOException;
2627
import java.util.Map;
2728
import org.junit.Before;
@@ -51,8 +52,38 @@ public boolean mayBeSlow() {
5152
}
5253

5354
@Override
54-
public void shutdown() {
55-
// Intentionally left empty.
55+
public int refCnt() {
56+
return 0;
57+
}
58+
59+
@Override
60+
public ReferenceCounted retain() {
61+
return this;
62+
}
63+
64+
@Override
65+
public ReferenceCounted retain(int i) {
66+
return this;
67+
}
68+
69+
@Override
70+
public ReferenceCounted touch() {
71+
return this;
72+
}
73+
74+
@Override
75+
public ReferenceCounted touch(Object o) {
76+
return this;
77+
}
78+
79+
@Override
80+
public boolean release() {
81+
return false;
82+
}
83+
84+
@Override
85+
public boolean release(int i) {
86+
return false;
5687
}
5788
};
5889
uploaderFactories =

‎src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ java_test(
2626
"//third_party:guava",
2727
"//third_party:junit4",
2828
"//third_party:mockito",
29+
"//third_party:netty",
2930
"//third_party:truth",
3031
"//third_party/protobuf:protobuf_java",
3132
"//third_party/protobuf:protobuf_java_util",

‎src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@
4343
import com.google.devtools.build.lib.buildeventstream.PathConverter.FileUriPathConverter;
4444
import com.google.devtools.build.lib.vfs.Path;
4545
import com.google.devtools.common.options.Options;
46+
import io.netty.util.AbstractReferenceCounted;
47+
import io.netty.util.ReferenceCounted;
4648
import java.io.BufferedOutputStream;
4749
import java.io.File;
4850
import java.io.FileInputStream;
@@ -137,7 +139,7 @@ public void testCancelledUpload() throws Exception {
137139

138140
BuildEventArtifactUploader uploader =
139141
Mockito.spy(
140-
new BuildEventArtifactUploader() {
142+
new BuildEventArtifactUploaderWithRefCounting() {
141143
@Override
142144
public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
143145
return Futures.immediateCancelledFuture();
@@ -147,9 +149,6 @@ public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
147149
public boolean mayBeSlow() {
148150
return false;
149151
}
150-
151-
@Override
152-
public void shutdown() {}
153152
});
154153

155154
File output = tmp.newFile();
@@ -248,7 +247,7 @@ public void testWritesWithUploadDelays() throws Exception {
248247

249248
BuildEventArtifactUploader uploader =
250249
Mockito.spy(
251-
new BuildEventArtifactUploader() {
250+
new BuildEventArtifactUploaderWithRefCounting() {
252251
@Override
253252
public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
254253
if (files.containsKey(file1)) {
@@ -261,11 +260,6 @@ public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
261260
public boolean mayBeSlow() {
262261
return true;
263262
}
264-
265-
@Override
266-
public void shutdown() {
267-
// Intentionally left empty.
268-
}
269263
});
270264
File output = tmp.newFile();
271265
BufferedOutputStream outputStream =
@@ -284,7 +278,7 @@ public void shutdown() {
284278
assertThat(in.available()).isEqualTo(0);
285279
}
286280

287-
verify(uploader).shutdown();
281+
verify(uploader).release();
288282
}
289283

290284
/** Regression test for b/207287675 */
@@ -296,7 +290,7 @@ public void testHandlesDuplicateFiles() throws Exception {
296290

297291
BuildEventArtifactUploader uploader =
298292
Mockito.spy(
299-
new BuildEventArtifactUploader() {
293+
new BuildEventArtifactUploaderWithRefCounting() {
300294
@Override
301295
public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
302296
return Futures.immediateFuture(new FileUriPathConverter());
@@ -306,11 +300,6 @@ public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
306300
public boolean mayBeSlow() {
307301
return false;
308302
}
309-
310-
@Override
311-
public void shutdown() {
312-
// Intentionally left empty.
313-
}
314303
});
315304
File output = tmp.newFile();
316305
BufferedOutputStream outputStream =
@@ -338,7 +327,7 @@ public void testCloseWaitsForWritesToFinish() throws Exception {
338327
SettableFuture<PathConverter> upload = SettableFuture.create();
339328
BuildEventArtifactUploader uploader =
340329
Mockito.spy(
341-
new BuildEventArtifactUploader() {
330+
new BuildEventArtifactUploaderWithRefCounting() {
342331
@Override
343332
public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
344333
return upload;
@@ -348,11 +337,6 @@ public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
348337
public boolean mayBeSlow() {
349338
return false;
350339
}
351-
352-
@Override
353-
public void shutdown() {
354-
// Intentionally left empty.
355-
}
356340
});
357341

358342
File output = tmp.newFile();
@@ -373,7 +357,7 @@ public void shutdown() {
373357
assertThat(in.available()).isEqualTo(0);
374358
}
375359

376-
verify(uploader).shutdown();
360+
verify(uploader).release();
377361
}
378362

379363
private static class WithLocalFilesEvent implements BuildEvent {
@@ -418,4 +402,16 @@ public Collection<BuildEventId> getChildrenEvents() {
418402
return ImmutableList.of(BuildEventIdUtil.progressId(id + 1));
419403
}
420404
}
405+
406+
private abstract static class BuildEventArtifactUploaderWithRefCounting
407+
extends AbstractReferenceCounted implements BuildEventArtifactUploader {
408+
409+
@Override
410+
protected void deallocate() {}
411+
412+
@Override
413+
public ReferenceCounted touch(Object o) {
414+
return this;
415+
}
416+
}
421417
}

‎src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ public void uploadsShouldWork() throws Exception {
181181
.isEqualTo("bytestream://localhost/instance/blobs/" + hash + "/" + size);
182182
}
183183

184-
artifactUploader.shutdown();
184+
artifactUploader.release();
185185

186186
assertThat(uploader.refCnt()).isEqualTo(0);
187187
assertThat(refCntChannel.isShutdown()).isTrue();
@@ -198,7 +198,7 @@ public void testUploadDirectoryDoesNotCrash() throws Exception {
198198

199199
PathConverter pathConverter = artifactUploader.upload(filesToUpload).get();
200200
assertThat(pathConverter.apply(dir)).isNull();
201-
artifactUploader.shutdown();
201+
artifactUploader.release();
202202
}
203203

204204
@Test
@@ -267,7 +267,7 @@ public void onCompleted() {
267267
assertThat(e.getCause().getCause()).isInstanceOf(StatusRuntimeException.class);
268268
assertThat(Status.fromThrowable(e).getCode()).isEqualTo(Status.CANCELLED.getCode());
269269

270-
artifactUploader.shutdown();
270+
artifactUploader.release();
271271

272272
assertThat(uploader.refCnt()).isEqualTo(0);
273273
assertThat(refCntChannel.isShutdown()).isTrue();

0 commit comments

Comments
 (0)