commit e22570a2a3df48220b18bdc28dc78fa16801d205
author Alexei Frolov <frolv@google.com> Wed Nov 18 14:56:18 2020 -0800
committer CQ Bot Account <pigweed-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Nov 19 00:28:02 2020 +0000
tree ed250d652d188079ba7af07aaa0630869b07ea47
parent 298d309ecf4434087197255504bbd69b86a92c95

pw_rpc: Return FailedPrecondition when writing to closed ServerWriter

If a ServerWriter is closed, its AcquirePayloadBuffer function returns
an empty span. The nanopb ServerWriter would then try to encode into
this span and fail, returning a cryptic INTERNAL status. This updates
the nanopb writer to correctly return FAILED_PRECONDITION if Write is
called when it is closed.

Change-Id: I30c70f0ee2a7f7c6f44bb8aded9aafa2045b99a4
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/25260
Commit-Queue: Alexei Frolov <frolv@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>

pw_rpc/nanopb/nanopb_method_test.cc

diff --git a/pw_rpc/nanopb/nanopb_method_test.cc b/pw_rpc/nanopb/nanopb_method_test.cc
index a02cf89..fa0072c 100644
--- a/pw_rpc/nanopb/nanopb_method_test.cc
+++ b/pw_rpc/nanopb/nanopb_method_test.cc
@@ -161,6 +161,17 @@
                         encoded.value().size()));
 }
 
+TEST(NanopbMethod, ServerWriter_WriteWhenClosed_ReturnsFailedPrecondition) {
+  const NanopbMethod& method =
+      std::get<2>(FakeService::kMethods).nanopb_method();
+  ServerContextForTest<FakeService> context(method);
+
+  method.Invoke(context.get(), context.packet({}));
+
+  last_writer.Finish();
+  EXPECT_TRUE(last_writer.Write({.value = 100}).IsFailedPrecondition());
+}
+
 TEST(NanopbMethod,
      ServerStreamingRpc_ServerWriterBufferTooSmall_InternalError) {
   const NanopbMethod& method =

pw_rpc/nanopb/public/pw_rpc/internal/nanopb_method.h

diff --git a/pw_rpc/nanopb/public/pw_rpc/internal/nanopb_method.h b/pw_rpc/nanopb/public/pw_rpc/internal/nanopb_method.h
index 78b798c..1808b34 100644
--- a/pw_rpc/nanopb/public/pw_rpc/internal/nanopb_method.h
+++ b/pw_rpc/nanopb/public/pw_rpc/internal/nanopb_method.h
@@ -293,6 +293,10 @@
 
 template <typename T>
 Status ServerWriter<T>::Write(const T& response) {
+  if (!open()) {
+    return Status::FailedPrecondition();
+  }
+
   std::span<std::byte> buffer = AcquirePayloadBuffer();
 
   if (auto result =

pw_rpc/raw/raw_method.cc

diff --git a/pw_rpc/raw/raw_method.cc b/pw_rpc/raw/raw_method.cc
index b23ff5b..4386aba 100644
--- a/pw_rpc/raw/raw_method.cc
+++ b/pw_rpc/raw/raw_method.cc
@@ -28,6 +28,10 @@
 }
 
 Status RawServerWriter::Write(ConstByteSpan response) {
+  if (!open()) {
+    return Status::FailedPrecondition();
+  }
+
   if (buffer().Contains(response)) {
     return ReleasePayloadBuffer(response);
   }

pw_rpc/raw/raw_method_test.cc

diff --git a/pw_rpc/raw/raw_method_test.cc b/pw_rpc/raw/raw_method_test.cc
index 9b03402..7679660 100644
--- a/pw_rpc/raw/raw_method_test.cc
+++ b/pw_rpc/raw/raw_method_test.cc
@@ -163,6 +163,17 @@
   EXPECT_EQ(packet.status(), Status::Ok());
 }
 
+TEST(RawServerWriter, Write_Closed_ReturnsFailedPrecondition) {
+  const RawMethod& method = std::get<1>(FakeService::kMethods).raw_method();
+  ServerContextForTest<FakeService, 16> context(method);
+
+  method.Invoke(context.get(), context.packet({}));
+
+  last_writer.Finish();
+  constexpr auto data = bytes::Array<0x0d, 0x06, 0xf0, 0x0d>();
+  EXPECT_EQ(last_writer.Write(data), Status::FailedPrecondition());
+}
+
 TEST(RawServerWriter, Write_BufferTooSmall_ReturnsOutOfRange) {
   const RawMethod& method = std::get<1>(FakeService::kMethods).raw_method();
   ServerContextForTest<FakeService, 16> context(method);