commit b0c5c998e1b3e18898eeb5e1a54a56e8d8c2d714
author Alexei Frolov <frolv@google.com> Thu Jan 06 13:24:43 2022 -0800
committer CQ Bot Account <pigweed-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri Jan 07 18:32:15 2022 +0000
tree 2d9d66d70ee4806fa480d1a7de4899e45de6bdaf
parent 0bf1eb95496db6eac66c6d547060f8a09cee3161

pw_transfer: Don't call Finish() on inactive transfers in the C++ client

This adds the same bugfix as pwrev/75980 to the transfer client.

These bugs are indicative of a design issue in the transfer client;
inactive transfers contexts should not be invoked. This will be fixed
in a later change -- a tracking bug is created.

Change-Id: I0ffc45d3c9343587fd4574ce13cd8e2762b612ea
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/77200
Pigweed-Auto-Submit: Alexei Frolov <frolv@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>

pw_transfer/client.cc

diff --git a/pw_transfer/client.cc b/pw_transfer/client.cc
index 80d060c..1a6ffb1 100644
--- a/pw_transfer/client.cc
+++ b/pw_transfer/client.cc
@@ -110,7 +110,10 @@
   return context->InitiateTransfer(max_parameters_);
 }
 
-Client::ClientContext* Client::GetActiveTransfer(uint32_t transfer_id) {
+// TODO(pwbug/592): This function should be updated to only return active
+// transfers. Calling ReadChunkData() / Finish() on inactive transfers is
+// unintuitive and has led to several bugs where not all cases are handled.
+Client::ClientContext* Client::GetTransferById(uint32_t transfer_id) {
   std::lock_guard lock(transfer_context_mutex_);
   auto it =
       std::find_if(transfer_contexts_.begin(),
@@ -133,7 +136,7 @@
     return;
   }
 
-  ClientContext* ctx = GetActiveTransfer(chunk.transfer_id);
+  ClientContext* ctx = GetTransferById(chunk.transfer_id);
   if (ctx == nullptr) {
     // TODO(frolv): Handle this error case.
     return;
@@ -143,7 +146,10 @@
     PW_LOG_ERROR(
         "Received a read chunk for transfer %u, but it is a write transfer",
         static_cast<unsigned>(ctx->transfer_id()));
-    ctx->Finish(Status::Internal());
+    if (ctx->active()) {
+      // TODO(pwbug/592): Remove the active() check.
+      ctx->Finish(Status::Internal());
+    }
     return;
   }
 
@@ -151,14 +157,20 @@
     PW_LOG_ERROR(
         "Received a write chunk for transfer %u, but it is a read transfer",
         static_cast<unsigned>(ctx->transfer_id()));
-    ctx->Finish(Status::Internal());
+    if (ctx->active()) {
+      // TODO(pwbug/592): Remove the active() check.
+      ctx->Finish(Status::Internal());
+    }
     return;
   }
 
-  if (chunk.status.has_value()) {
+  if (chunk.status.has_value() && ctx->active()) {
     // A status field indicates that the transfer has finished.
+    //
     // TODO(frolv): This is invoked from the RPC client thread -- should it be
     // run in the work queue instead?
+    //
+    // TODO(pwbug/592): Remove the active() check.
     ctx->Finish(chunk.status.value());
     return;
   }

pw_transfer/public/pw_transfer/client.h

diff --git a/pw_transfer/public/pw_transfer/client.h b/pw_transfer/public/pw_transfer/client.h
index 4501103..12f43a8 100644
--- a/pw_transfer/public/pw_transfer/client.h
+++ b/pw_transfer/public/pw_transfer/client.h
@@ -97,7 +97,7 @@
                           CompletionFunc&& on_completion,
                           chrono::SystemClock::duration timeout);
 
-  ClientContext* GetActiveTransfer(uint32_t transfer_id);
+  ClientContext* GetTransferById(uint32_t transfer_id);
 
   // Function called when a chunk is received, from the context of the RPC
   // client thread.