From 5a3cff01acf330250e18e4cb0a502cd1ed871dca Mon Sep 17 00:00:00 2001 From: Wyatt Hepler Date: Wed, 4 May 2022 15:25:20 -0700 Subject: [PATCH] pw_transfer: Remove chunk size adjustment from Java client The chunk size adjustment feature is an unusual, low-level feature that is not in use. Remove the feature to clean up the code. Change-Id: I8510362c659f873ce20cd960fd0b2c6b1f4df4e4 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/93604 Pigweed-Auto-Submit: Wyatt Hepler Reviewed-by: Alexei Frolov Commit-Queue: Auto-Submit --- .../main/dev/pigweed/pw_transfer/Manager.java | 27 +------- .../pigweed/pw_transfer/WriteTransfer.java | 28 ++------- .../dev/pigweed/pw_transfer/ManagerTest.java | 62 ------------------- 3 files changed, 7 insertions(+), 110 deletions(-) diff --git a/pw_transfer/java/main/dev/pigweed/pw_transfer/Manager.java b/pw_transfer/java/main/dev/pigweed/pw_transfer/Manager.java index c745e041a7..b64e2859f4 100644 --- a/pw_transfer/java/main/dev/pigweed/pw_transfer/Manager.java +++ b/pw_transfer/java/main/dev/pigweed/pw_transfer/Manager.java @@ -60,16 +60,6 @@ public class Manager { @Nullable private Call.ClientStreaming readStream = null; @Nullable private Call.ClientStreaming writeStream = null; - /** - * Callback for adjusting the size of a pw_transfer write chunk. - * - *

If the adjusted size is 0 or negative, the transfer is aborted. If the adjusted size is - * larger than the chunk, the chunk size remains unchanged. - */ - public interface ChunkSizeAdjuster { - int getAdjustedChunkSize(ByteString chunkData, int maxChunkSizeBytes); - } - /** * Creates a new Manager for sending and receiving data with pw_transfer. * @@ -110,23 +100,13 @@ public ListenableFuture write(int resourceId, byte[] data) { /** * Writes data to the specified transfer resource, calling the progress * callback as data is sent. - */ - public ListenableFuture write( - int resourceId, byte[] data, Consumer progressCallback) { - return write(resourceId, data, progressCallback, (chunk, maxSize) -> chunk.size()); - } - /** - * Writes the data to the specified transfer resource. * * @param resourceId The ID of the resource to which to write * @param data the data to write * @param progressCallback called each time a packet is sent - * @param chunkSizeAdjustment callback to optionally reduce the number of bytes to send */ - public synchronized ListenableFuture write(int resourceId, - byte[] data, - Consumer progressCallback, - ChunkSizeAdjuster chunkSizeAdjustment) { + public synchronized ListenableFuture write( + int resourceId, byte[] data, Consumer progressCallback) { return startTransfer(writeTransfers, new WriteTransfer(resourceId, this::sendWriteChunk, @@ -137,8 +117,7 @@ public synchronized ListenableFuture write(int resourceId, maxRetries, data, progressCallback, - shouldAbortCallback, - chunkSizeAdjustment)); + shouldAbortCallback)); } /** Reads the data from the given transfer resource ID. */ diff --git a/pw_transfer/java/main/dev/pigweed/pw_transfer/WriteTransfer.java b/pw_transfer/java/main/dev/pigweed/pw_transfer/WriteTransfer.java index c4901c81a4..40feed24b3 100644 --- a/pw_transfer/java/main/dev/pigweed/pw_transfer/WriteTransfer.java +++ b/pw_transfer/java/main/dev/pigweed/pw_transfer/WriteTransfer.java @@ -21,7 +21,6 @@ import com.google.protobuf.ByteString; import dev.pigweed.pw_log.Logger; import dev.pigweed.pw_rpc.Status; -import dev.pigweed.pw_transfer.Manager.ChunkSizeAdjuster; import java.util.Timer; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.BooleanSupplier; @@ -43,7 +42,6 @@ class WriteTransfer extends Transfer { private Chunk lastChunk; private final byte[] data; - private final ChunkSizeAdjuster chunkSizeAdjustment; protected WriteTransfer(int id, ChunkSender sendChunk, @@ -54,8 +52,7 @@ protected WriteTransfer(int id, int maxRetries, byte[] data, Consumer progressCallback, - BooleanSupplier shouldAbortCallback, - ChunkSizeAdjuster chunkSizeAdjustment) { + BooleanSupplier shouldAbortCallback) { super(id, sendChunk, endTransfer, @@ -66,8 +63,6 @@ protected WriteTransfer(int id, progressCallback, shouldAbortCallback); this.data = data; - this.chunkSizeAdjustment = chunkSizeAdjustment; - this.lastChunk = getInitialChunk(); } @@ -172,28 +167,13 @@ private synchronized boolean doHandleDataChunk(Chunk chunk) { ByteString chunkData = ByteString.copyFrom( data, sentOffset, min(windowEndOffset - sentOffset, maxChunkSizeBytes)); - // Apply the chunk size adjustment. The returned chunk size is capped at chunkData.size(). - // Sending 0 bytes of an non-empty chunk is invalid and aborts the transfer. - final int newChunkSize = min( - chunkSizeAdjustment.getAdjustedChunkSize(chunkData, maxChunkSizeBytes), chunkData.size()); - if ((!chunkData.isEmpty() && newChunkSize == 0) || newChunkSize < 0) { - logger.atWarning().log( - "Transfer %d: attempted to adjust %d B chunk to %d B; aborting transfer", - getId(), - chunkData.size(), - newChunkSize); - sendFinalChunk(Status.INVALID_ARGUMENT); - return true; - } - logger.atFiner().log( - "Transfer %d: sending %d B chunk (adjusted from %d B) with max size of %d", + logger.atFiner().log("Transfer %d: sending bytes %d-%d (%d B chunk, max size %d B)", getId(), - newChunkSize, + sentOffset, + sentOffset + chunkData.size() - 1, chunkData.size(), maxChunkSizeBytes); - chunkData = chunkData.substring(0, newChunkSize); - chunkToSend = buildDataChunk(chunkData); // If there's a timeout, resending this will trigger a transfer parameters update. diff --git a/pw_transfer/java/test/dev/pigweed/pw_transfer/ManagerTest.java b/pw_transfer/java/test/dev/pigweed/pw_transfer/ManagerTest.java index 3fef9fdc4f..a9c9fe734d 100644 --- a/pw_transfer/java/test/dev/pigweed/pw_transfer/ManagerTest.java +++ b/pw_transfer/java/test/dev/pigweed/pw_transfer/ManagerTest.java @@ -586,68 +586,6 @@ public void write_severalChunks() throws Exception { assertThat(future.get()).isNull(); // Ensure that no exceptions are thrown. } - @Test - public void write_adjustChunkSize() throws Exception { - ListenableFuture future = // Always request 30-byte chunks - manager.write(ID, TEST_DATA_100B.toByteArray(), progress -> {}, (chunk, maxSize) -> 30); - - assertThat(lastChunks()).containsExactly(initialWriteChunk(ID, ID, TEST_DATA_100B.size())); - - receiveWriteChunks(newChunk(Chunk.Type.PARAMETERS_RETRANSMIT, ID) - .setOffset(0) - .setPendingBytes(1024) - .setMaxChunkSizeBytes(100)); - - assertThat(lastChunks()) - .containsExactly( - newChunk(Chunk.Type.TRANSFER_DATA, ID).setOffset(0).setData(range(0, 30)).build(), - newChunk(Chunk.Type.TRANSFER_DATA, ID).setOffset(30).setData(range(30, 60)).build(), - newChunk(Chunk.Type.TRANSFER_DATA, ID).setOffset(60).setData(range(60, 90)).build(), - newChunk(Chunk.Type.TRANSFER_DATA, ID) - .setOffset(90) - .setData(range(90, 100)) - .setRemainingBytes(0) - .build()); - - receiveWriteChunks(finalChunk(ID, Status.OK)); - - assertThat(future.get()).isNull(); // Ensure that no exceptions are thrown. - } - - @Test - public void write_adjustChunkSize_zeroLengthAdjustment_abortsTransfer() { - ListenableFuture future = // Always request 0-byte chunks, which is invalid. - manager.write(ID, TEST_DATA_100B.toByteArray(), progress -> {}, (chunk, maxSize) -> 0); - - assertThat(lastChunks()).containsExactly(initialWriteChunk(ID, ID, TEST_DATA_100B.size())); - - receiveWriteChunks(newChunk(Chunk.Type.PARAMETERS_RETRANSMIT, ID) - .setOffset(0) - .setPendingBytes(1024) - .setMaxChunkSizeBytes(100)); - - ExecutionException thrown = assertThrows(ExecutionException.class, future::get); - assertThat(thrown).hasCauseThat().isInstanceOf(TransferError.class); - assertThat(((TransferError) thrown.getCause()).status()).isEqualTo(Status.INVALID_ARGUMENT); - } - - @Test - public void write_adjustChunkSize_negativeAdjustment_abortsTransfer() { - ListenableFuture future = // Always request negative chunks, which is invalid. - manager.write(ID, TEST_DATA_100B.toByteArray(), progress -> {}, (chunk, maxSize) -> - 1); - - assertThat(lastChunks()).containsExactly(initialWriteChunk(ID, ID, TEST_DATA_100B.size())); - - receiveWriteChunks(newChunk(Chunk.Type.PARAMETERS_RETRANSMIT, ID) - .setOffset(0) - .setPendingBytes(1024) - .setMaxChunkSizeBytes(100)); - - ExecutionException thrown = assertThrows(ExecutionException.class, future::get); - assertThat(thrown).hasCauseThat().isInstanceOf(TransferError.class); - assertThat(((TransferError) thrown.getCause()).status()).isEqualTo(Status.INVALID_ARGUMENT); - } - @Test public void write_parametersContinue() throws Exception { ListenableFuture future = manager.write(ID, TEST_DATA_100B.toByteArray());