Skip to content

Commit

Permalink
pw_transfer: Remove chunk size adjustment from Java client
Browse files Browse the repository at this point in the history 
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 <hepler@google.com>
Reviewed-by: Alexei Frolov <frolv@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
  • Loading branch information
@255
255 authored and CQ Bot Account committed May 5, 2022
1 parent 3c4b7a2 commit 5a3cff0
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 110 deletions.
27 changes: 3 additions & 24 deletions pw_transfer/java/main/dev/pigweed/pw_transfer/Manager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,6 @@ public class Manager {
@Nullable private Call.ClientStreaming<Chunk> readStream = null;
@Nullable private Call.ClientStreaming<Chunk> writeStream = null;

/**
* Callback for adjusting the size of a pw_transfer write chunk.
*
* <p>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.
*
Expand Down Expand Up @@ -110,23 +100,13 @@ public ListenableFuture<Void> write(int resourceId, byte[] data) {
/**
* Writes data to the specified transfer resource, calling the progress
* callback as data is sent.
*/
public ListenableFuture<Void> write(
int resourceId, byte[] data, Consumer<TransferProgress> 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<Void> write(int resourceId,
byte[] data,
Consumer<TransferProgress> progressCallback,
ChunkSizeAdjuster chunkSizeAdjustment) {
public synchronized ListenableFuture<Void> write(
int resourceId, byte[] data, Consumer<TransferProgress> progressCallback) {
return startTransfer(writeTransfers,
new WriteTransfer(resourceId,
this::sendWriteChunk,
Expand All @@ -137,8 +117,7 @@ public synchronized ListenableFuture<Void> write(int resourceId,
maxRetries,
data,
progressCallback,
shouldAbortCallback,
chunkSizeAdjustment));
shouldAbortCallback));
}

/** Reads the data from the given transfer resource ID. */
Expand Down
28 changes: 4 additions & 24 deletions pw_transfer/java/main/dev/pigweed/pw_transfer/WriteTransfer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -43,7 +42,6 @@ class WriteTransfer extends Transfer<Void> {
private Chunk lastChunk;

private final byte[] data;
private final ChunkSizeAdjuster chunkSizeAdjustment;

protected WriteTransfer(int id,
ChunkSender sendChunk,
Expand All @@ -54,8 +52,7 @@ protected WriteTransfer(int id,
int maxRetries,
byte[] data,
Consumer<TransferProgress> progressCallback,
BooleanSupplier shouldAbortCallback,
ChunkSizeAdjuster chunkSizeAdjustment) {
BooleanSupplier shouldAbortCallback) {
super(id,
sendChunk,
endTransfer,
Expand All @@ -66,8 +63,6 @@ protected WriteTransfer(int id,
progressCallback,
shouldAbortCallback);
this.data = data;
this.chunkSizeAdjustment = chunkSizeAdjustment;

this.lastChunk = getInitialChunk();
}

Expand Down Expand Up @@ -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.
Expand Down
62 changes: 0 additions & 62 deletions pw_transfer/java/test/dev/pigweed/pw_transfer/ManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Void> 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<Void> 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<Void> 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<Void> future = manager.write(ID, TEST_DATA_100B.toByteArray());
Expand Down

0 comments on commit 5a3cff0

Please sign in to comment.