From 0c9266270ee2d3014254983ca63312017c9a532d Mon Sep 17 00:00:00 2001
From: Krzysztof Witukiewicz <krzysztof.witukiewicz@mgm-tp.com>
Date: Thu, 30 Jan 2025 15:38:34 +0100
Subject: [PATCH] OZG-7262 OZG-7627 Don't call sendChunks() after exception
---
.../GrpcBinaryFileServerDownloader.java | 31 ++-
.../GrpcBinaryFileServerDownloaderTest.java | 239 +++++++-----------
2 files changed, 103 insertions(+), 167 deletions(-)
diff --git a/ozgcloud-common-lib/src/main/java/de/ozgcloud/common/binaryfile/GrpcBinaryFileServerDownloader.java b/ozgcloud-common-lib/src/main/java/de/ozgcloud/common/binaryfile/GrpcBinaryFileServerDownloader.java
index b2d2df3..8acac69 100644
--- a/ozgcloud-common-lib/src/main/java/de/ozgcloud/common/binaryfile/GrpcBinaryFileServerDownloader.java
+++ b/ozgcloud-common-lib/src/main/java/de/ozgcloud/common/binaryfile/GrpcBinaryFileServerDownloader.java
@@ -57,7 +57,7 @@ public class GrpcBinaryFileServerDownloader<T> {
private final AtomicBoolean started = new AtomicBoolean(false);
private final AtomicBoolean downloadFinished = new AtomicBoolean(false);
private final AtomicBoolean requestFinished = new AtomicBoolean(false);
- private final AtomicReference<TechnicalException> error = new AtomicReference<>();
+ private final AtomicReference<TechnicalException> downloadError = new AtomicReference<>();
private PipedInputStream inputStream;
private PipedOutputStream outputStream;
@@ -107,11 +107,12 @@ public class GrpcBinaryFileServerDownloader<T> {
void startDownload() {
try {
doDownload();
+ sendChunks();
} catch (Exception e) {
- error.set(new TechnicalException("Error while downloading file contents", e));
+ LOG.error("Error while downloading file contents", e);
+ downloadError.set(new TechnicalException("Error while downloading file contents", e));
} finally {
closeOutputStream();
- sendChunks();
}
}
@@ -136,10 +137,6 @@ public class GrpcBinaryFileServerDownloader<T> {
}
int bytesRead;
while (isReady()) {
- if (Objects.nonNull(error.get())) {
- completeRequestWithError(error.get());
- break;
- }
if ((bytesRead = inputStream.read(buffer)) == -1) {
tryCompleteRequest();
break;
@@ -154,26 +151,28 @@ public class GrpcBinaryFileServerDownloader<T> {
}
void tryCompleteRequest() {
- if (shouldCompleteRequest()) {
- completeRequest();
+ if (Objects.nonNull(downloadError.get())) {
+ throw downloadError.get();
+ } else if (downloadFinished.get()) {
+ completeRequestNormally();
}
}
- boolean shouldCompleteRequest() {
- return downloadFinished.get() && requestFinished.compareAndSet(false, true);
+ void completeRequestWithError(TechnicalException e) {
+ LOG.debug("Complete download request with error");
+ finishRequest();
+ throw e;
}
- void completeRequest() {
+ void completeRequestNormally() {
LOG.debug("Complete download request");
- closeInputStream();
+ finishRequest();
callObserver.onCompleted();
}
- void completeRequestWithError(TechnicalException e) {
- LOG.debug("Complete download request with error", e);
+ private void finishRequest() {
requestFinished.set(true);
closeInputStream();
- callObserver.onError(e);
}
void closeOutputStream() {
diff --git a/ozgcloud-common-lib/src/test/java/de/ozgcloud/common/binaryfile/GrpcBinaryFileServerDownloaderTest.java b/ozgcloud-common-lib/src/test/java/de/ozgcloud/common/binaryfile/GrpcBinaryFileServerDownloaderTest.java
index 81da69e..8c3ad5a 100644
--- a/ozgcloud-common-lib/src/test/java/de/ozgcloud/common/binaryfile/GrpcBinaryFileServerDownloaderTest.java
+++ b/ozgcloud-common-lib/src/test/java/de/ozgcloud/common/binaryfile/GrpcBinaryFileServerDownloaderTest.java
@@ -229,7 +229,7 @@ class GrpcBinaryFileServerDownloaderTest {
@Test
void shouldErrorBeInitiallyNull() {
- assertThat(getError()).isNull();
+ assertThat(getDownloadError()).isNull();
}
@Nested
@@ -256,7 +256,6 @@ class GrpcBinaryFileServerDownloaderTest {
}
@Test
- @DisplayName("should send chunks here to not wait for callObserver to change its ready status")
void shouldSendChunks() {
downloader.startDownload();
@@ -278,7 +277,7 @@ class GrpcBinaryFileServerDownloaderTest {
void shouldSetError() {
downloader.startDownload();
- assertThat(getError()).isInstanceOf(TechnicalException.class).hasCause(exception);
+ assertThat(getDownloadError()).isInstanceOf(TechnicalException.class).hasCause(exception);
}
@SneakyThrows
@@ -288,14 +287,6 @@ class GrpcBinaryFileServerDownloaderTest {
verify(outputStream).close();
}
-
- @Test
- @DisplayName("should send chunks here to communicate error to callObserver")
- void shouldSendChunks() {
- downloader.startDownload();
-
- verify(downloader).sendChunks();
- }
}
}
@@ -362,6 +353,7 @@ class GrpcBinaryFileServerDownloaderTest {
@BeforeEach
void init() {
doThrow(exception).when(downloader).doSendChunks();
+ doNothing().when(downloader).completeRequestWithError(any());
}
@Test
@@ -416,87 +408,52 @@ class GrpcBinaryFileServerDownloaderTest {
@Nested
class OnReady {
+ @Mock
+ private PipedInputStream inputStream;
+ @Captor
+ private ArgumentCaptor<ByteString> byteStringCaptor;
+
+ private final int readBytes = 20;
+ private final byte[] buffer = new byte[readBytes];
+ private final GrpcResponseDummy grpcResponseDummy = new GrpcResponseDummy();
+
+ @SneakyThrows
@BeforeEach
- void init() {
+ void mock() {
+ doNothing().when(downloader).tryCompleteRequest();
when(callObserver.isReady()).thenReturn(true);
+ when(inputStream.read(any())).thenReturn(readBytes, -1);
+ setInputStreamField(inputStream);
+ new Random().nextBytes(buffer);
+ ReflectionTestUtils.setField(downloader, "buffer", buffer);
}
- @Nested
- class OnHasError {
+ @Test
+ void shouldCallChunkBuilder() {
+ doSendChunks();
- private final TechnicalException exception = new TechnicalException("error");
+ verify(chunkBuilder).apply(byteStringCaptor.capture());
+ assertThat(byteStringCaptor.getValue().toByteArray()).isEqualTo(buffer);
+ }
- @BeforeEach
- void init() {
- setErrorField(exception);
- doNothing().when(downloader).completeRequestWithError(any());
- }
+ @DisplayName("should send next chunk if callObserver is ready and stream already received data")
+ @Test
+ void shouldCallOnNext() {
+ when(chunkBuilder.apply(any())).thenReturn(grpcResponseDummy);
- @Test
- void shouldOnlyCallIsReadyOnObserver() {
- doSendChunks();
+ doSendChunks();
- verify(callObserver).isReady();
- verifyNoMoreInteractions(callObserver);
- }
+ verify(callObserver).onNext(grpcResponseDummy);
+ }
- @Test
- void shouldCompleteRequestWithError() {
- doSendChunks();
+ @DisplayName("should call complete grpc stream if download has finished and stream has no data left")
+ @Test
+ void shouldTryCompleteRequest() {
+ setDownloadFinishedField(true);
- verify(downloader).completeRequestWithError(exception);
- }
- }
+ doSendChunks();
- @Nested
- class OnHasNoError {
- @Mock
- private PipedInputStream inputStream;
- @Captor
- private ArgumentCaptor<ByteString> byteStringCaptor;
-
- private final int readBytes = 20;
- private final byte[] buffer = new byte[readBytes];
- private final GrpcResponseDummy grpcResponseDummy = new GrpcResponseDummy();
-
- @SneakyThrows
- @BeforeEach
- void mock() {
- doNothing().when(downloader).tryCompleteRequest();
- when(callObserver.isReady()).thenReturn(true);
- when(inputStream.read(any())).thenReturn(readBytes, -1);
- setInputStreamField(inputStream);
- new Random().nextBytes(buffer);
- ReflectionTestUtils.setField(downloader, "buffer", buffer);
- }
-
- @Test
- void shouldCallChunkBuilder() {
- doSendChunks();
-
- verify(chunkBuilder).apply(byteStringCaptor.capture());
- assertThat(byteStringCaptor.getValue().toByteArray()).isEqualTo(buffer);
- }
-
- @DisplayName("should send next chunk if callObserver is ready and stream already received data")
- @Test
- void shouldCallOnNext() {
- when(chunkBuilder.apply(any())).thenReturn(grpcResponseDummy);
-
- doSendChunks();
-
- verify(callObserver).onNext(grpcResponseDummy);
- }
-
- @DisplayName("should call complete grpc stream if download has finished and stream has no data left")
- @Test
- void shouldTryCompleteRequest() {
- setDownloadFinishedField(true);
-
- doSendChunks();
-
- verify(downloader).tryCompleteRequest();
- }
+ verify(downloader).tryCompleteRequest();
}
}
}
@@ -510,69 +467,48 @@ class GrpcBinaryFileServerDownloaderTest {
@Nested
class TestTryCompleteRequest {
- @Test
- void shouldCallShouldCompleteRequest() {
- downloader.tryCompleteRequest();
-
- verify(downloader).shouldCompleteRequest();
- }
-
- @Test
- void shouldCallCompleteRequest() {
- doReturn(true).when(downloader).shouldCompleteRequest();
-
- downloader.tryCompleteRequest();
-
- verify(downloader).completeRequest();
- }
+ @Nested
+ class OnError {
- @Test
- void shouldNotCallCompleteRequest() {
- doReturn(false).when(downloader).shouldCompleteRequest();
+ private final TechnicalException exception = new TechnicalException("error");
- downloader.tryCompleteRequest();
+ @BeforeEach
+ void init() {
+ setDownloadErrorField(exception);
+ }
- verify(downloader, never()).completeRequest();
+ @Test
+ void shouldThrowException() {
+ assertThatThrownBy(downloader::tryCompleteRequest).isSameAs(exception);
+ }
}
- }
-
- @Nested
- class TestShouldCompleteRequest {
@Nested
- class TestWhenDownloadFinished {
+ class OnDownloadFinished {
@BeforeEach
void init() {
setDownloadFinishedField(true);
+ doNothing().when(downloader).completeRequestNormally();
}
@Test
- void shouldReturnTrue() {
- var result = downloader.shouldCompleteRequest();
+ void shouldNotCompleteRequestWithError() {
+ downloader.tryCompleteRequest();
- assertThat(result).isTrue();
+ verify(downloader, never()).completeRequestWithError(any());
}
@Test
- void shouldReturnFalseIfRequestFinished() {
- setRequestFinishedField(true);
-
- var result = downloader.shouldCompleteRequest();
+ void shouldCompleteRequestNormally() {
+ downloader.tryCompleteRequest();
- assertThat(result).isFalse();
- }
-
- @Test
- void shouldUpdateRequestFinished() {
- downloader.shouldCompleteRequest();
-
- assertThat(getRequestFinished()).isTrue();
+ verify(downloader).completeRequestNormally();
}
}
@Nested
- class TestWhenDownloadRunning {
+ class OnDownloadNotFinished {
@BeforeEach
void init() {
@@ -580,45 +516,48 @@ class GrpcBinaryFileServerDownloaderTest {
}
@Test
- void shouldReturnFalse() {
- var result = downloader.shouldCompleteRequest();
+ void shouldNotCompleteRequestNormally() {
+ downloader.tryCompleteRequest();
- assertThat(result).isFalse();
+ verify(downloader, never()).completeRequestNormally();
}
@Test
- void shouldNotUpdateRequestFinished() {
- downloader.shouldCompleteRequest();
+ void shouldNotCompleteRequestWithError() {
+ downloader.tryCompleteRequest();
- assertThat(getRequestFinished()).isFalse();
+ verify(downloader, never()).completeRequestWithError(any());
}
}
}
@Nested
- class TestCompleteRequest {
-
- @Mock
- private PipedInputStream inputStream;
+ class TestCompleteRequestNormally {
@BeforeEach
- void mock() {
- setRequestFinishedField(false);
- setDownloadFinishedField(true);
- setInputStreamField(inputStream);
+ void init() {
+ doNothing().when(downloader).closeInputStream();
}
- @SneakyThrows
@Test
- void shouldCallCloseInputStream() {
- downloader.completeRequest();
+ void shouldSetRequestFinished() {
+ assertThat(getRequestFinished()).isFalse();
+
+ downloader.completeRequestNormally();
+
+ assertThat(getRequestFinished()).isTrue();
+ }
+
+ @Test
+ void shouldCloseInputStream() {
+ downloader.completeRequestNormally();
verify(downloader).closeInputStream();
}
@Test
- void shouldCallOnCompleted() {
- downloader.completeRequest();
+ void shouldNotifyObserver() {
+ downloader.completeRequestNormally();
verify(callObserver).onCompleted();
}
@@ -638,23 +577,21 @@ class GrpcBinaryFileServerDownloaderTest {
void shouldSetRequestFinished() {
assertThat(getRequestFinished()).isFalse();
- downloader.completeRequestWithError(error);
+ catchException(() -> downloader.completeRequestWithError(error));
assertThat(getRequestFinished()).isTrue();
}
@Test
void shouldCloseInputStream() {
- downloader.completeRequestWithError(error);
+ catchException(() -> downloader.completeRequestWithError(error));
verify(downloader).closeInputStream();
}
@Test
- void shouldNotifyCallObserver() {
- downloader.completeRequestWithError(error);
-
- verify(callObserver).onError(error);
+ void shouldThrowException() {
+ assertThatThrownBy(() -> downloader.completeRequestWithError(error)).isSameAs(error);
}
}
@@ -674,12 +611,12 @@ class GrpcBinaryFileServerDownloaderTest {
return ReflectionTestUtils.getField(downloader, "requestFinished", AtomicBoolean.class).get();
}
- private void setErrorField(TechnicalException error) {
- ReflectionTestUtils.setField(downloader, "error", new AtomicReference<>(error));
+ private void setDownloadErrorField(TechnicalException error) {
+ ReflectionTestUtils.setField(downloader, "downloadError", new AtomicReference<>(error));
}
- private TechnicalException getError() {
- return (TechnicalException) ReflectionTestUtils.getField(downloader, "error", AtomicReference.class).get();
+ private TechnicalException getDownloadError() {
+ return (TechnicalException) ReflectionTestUtils.getField(downloader, "downloadError", AtomicReference.class).get();
}
private void setDownloadFinishedField(boolean downloadFinished) {
--
GitLab