Refactor P2pCLusterPcpHandler and WifiLan.

PiperOrigin-RevId: 810525937
2026-09-14 14:46:12 -04:00 · 2025-09-23 11:56:05 -07:00
parent f98241826a
commit 6f92e238b0
4 changed files with 100 additions and 111 deletions
@@ -83,7 +83,8 @@ bool WifiLan::IsAvailable() const {
 bool WifiLan::IsAvailableLocked() const { return medium_.IsValid(); }

 ErrorOr<bool> WifiLan::StartAdvertising(const std::string& service_id,
-                                        NsdServiceInfo& nsd_service_info) {
+                                        NsdServiceInfo& nsd_service_info,
+                                        AcceptedConnectionCallback callback) {
  MutexLock lock(&mutex_);

  if (!IsAvailableLocked()) {
@@ -103,28 +104,25 @@ ErrorOr<bool> WifiLan::StartAdvertising(const std::string& service_id,
        << "Failed to WifiLan advertise because we're already advertising.";
    return {Error(OperationResultCode::CLIENT_WIFI_LAN_DUPLICATE_ADVERTISING)};
  }
-
-  if (!IsAcceptingConnectionsLocked(service_id)) {
-    LOG(INFO) << "Failed to turn on WifiLan advertising with nsd_service_info="
-              << &nsd_service_info
-              << ", service_name=" << nsd_service_info.GetServiceName()
-              << ", service_id=" << service_id
-              << ". Should accept connections before advertising.";
-    return {Error(OperationResultCode::
-                      CLIENT_DUPLICATE_ACCEPTING_LAN_CONNECTION_REQUEST)};
-  }
-
  nsd_service_info.SetServiceType(GenerateServiceType(service_id));
  const auto& it = server_sockets_.find(service_id);
  if (it != server_sockets_.end()) {
-    nsd_service_info.SetIPAddress(it->second.GetIPAddress());
    nsd_service_info.SetPort(it->second.GetPort());
+  } else {
+    int port = 0;
+    ErrorOr<int> port_result = StartAcceptingConnectionsLocked(
+        service_id, port, std::move(callback));
+    if (port_result.has_error()) {
+      return {port_result.error()};
+    }
+    nsd_service_info.SetPort(port_result.value());
  }
  if (!medium_.StartAdvertising(nsd_service_info)) {
    LOG(INFO) << "Failed to turn on WifiLan advertising with nsd_service_info="
              << &nsd_service_info
              << ", service_name=" << nsd_service_info.GetServiceName()
              << ", service_id=" << service_id;
+    StopAcceptingConnectionsLocked(service_id);
    return {Error(
        OperationResultCode::CONNECTIVITY_WIFI_LAN_START_ADVERTISING_FAILURE)};
  }
@@ -228,35 +226,12 @@ bool WifiLan::IsDiscoveringLocked(const std::string& service_id) {
  return discovering_info_.Existed(service_id);
 }

-ErrorOr<bool> WifiLan::StartAcceptingConnections(
-    const std::string& service_id, AcceptedConnectionCallback callback) {
-  MutexLock lock(&mutex_);
-
-  if (service_id.empty()) {
-    LOG(INFO) << "Refusing to start accepting WifiLan connections; "
-                 "service_id is empty.";
-    return {Error(OperationResultCode::NEARBY_LOCAL_CLIENT_STATE_WRONG)};
-  }
-
-  if (!IsAvailableLocked()) {
-    LOG(INFO) << "Can't start accepting WifiLan connections [service_id="
-              << service_id << "]; WifiLan not available.";
-    return {Error(
-        OperationResultCode::MEDIUM_UNAVAILABLE_WIFI_AWARE_NOT_AVAILABLE)};
-  }
-
-  if (IsAcceptingConnectionsLocked(service_id)) {
-    LOG(INFO) << "Refusing to start accepting WifiLan connections [service="
-              << service_id
-              << "]; WifiLan server is already in-progress with the same name.";
-    return {Error(OperationResultCode::
-                      CLIENT_DUPLICATE_ACCEPTING_LAN_CONNECTION_REQUEST)};
-  }
-
+ErrorOr<int> WifiLan::StartAcceptingConnectionsLocked(
+    const std::string& service_id, int port,
+    AcceptedConnectionCallback callback) {
  auto port_range = medium_.GetDynamicPortRange();
  // Generate an exact port here on server socket; if platform doesn't provide
  // range of port then assign 0 to let platform decide it.
-  int port = 0;
  if (port_range.has_value() &&
      (port_range->first > 0 && port_range->first <= 65535 &&
       port_range->second > 0 && port_range->second <= 65535 &&
@@ -289,6 +264,7 @@ ErrorOr<bool> WifiLan::StartAcceptingConnections(
          }
        });
  }
+  port = owned_server_socket.GetPort();
  // Start the accept loop on a dedicated thread - this stays alive and
  // listening for new incoming connections until StopAcceptingConnections() is
  // invoked.
@@ -358,12 +334,49 @@ ErrorOr<bool> WifiLan::StartAcceptingConnections(
        }
      });

+  return {port};
+}
+
+ErrorOr<bool> WifiLan::StartAcceptingConnections(
+    const std::string& service_id, AcceptedConnectionCallback callback) {
+  MutexLock lock(&mutex_);
+
+  if (service_id.empty()) {
+    LOG(INFO) << "Refusing to start accepting WifiLan connections; "
+                 "service_id is empty.";
+    return {Error(OperationResultCode::NEARBY_LOCAL_CLIENT_STATE_WRONG)};
+  }
+
+  if (!IsAvailableLocked()) {
+    LOG(INFO) << "Can't start accepting WifiLan connections [service_id="
+              << service_id << "]; WifiLan not available.";
+    return {Error(
+        OperationResultCode::MEDIUM_UNAVAILABLE_WIFI_AWARE_NOT_AVAILABLE)};
+  }
+
+  if (IsAcceptingConnectionsLocked(service_id)) {
+    LOG(INFO) << "Refusing to start accepting WifiLan connections [service="
+              << service_id
+              << "]; WifiLan server is already in-progress with the same name.";
+    return {Error(OperationResultCode::
+                      CLIENT_DUPLICATE_ACCEPTING_LAN_CONNECTION_REQUEST)};
+  }
+  ErrorOr<int> port = StartAcceptingConnectionsLocked(service_id, /*port=*/0,
+                                                      std::move(callback))
+                          .has_error();
+  if (port.has_error()) {
+    return {port.error()};
+  }
  return {true};
 }

 bool WifiLan::StopAcceptingConnections(const std::string& service_id) {
  MutexLock lock(&mutex_);

+  return StopAcceptingConnectionsLocked(service_id);
+}
+
+bool WifiLan::StopAcceptingConnectionsLocked(const std::string& service_id) {
  if (service_id.empty()) {
    LOG(INFO) << "Unable to stop accepting WifiLan connections because "
                 "the service_id is empty.";
@@ -55,7 +55,8 @@ class WifiLan {
  // then enables WifiLan advertising.
  // Returns true, if NsdServiceInfo is successfully set, and false otherwise.
  ErrorOr<bool> StartAdvertising(const std::string& service_id,
-                                 NsdServiceInfo& nsd_service_info)
+                                 NsdServiceInfo& nsd_service_info,
+                                 AcceptedConnectionCallback callback)
      ABSL_LOCKS_EXCLUDED(mutex_);

  // Disables WifiLan advertising.
@@ -183,6 +184,15 @@ class WifiLan {
  bool IsAcceptingConnectionsLocked(const std::string& service_id)
      ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_);

+  // Returns the port number of the server socket if successful, otherwise
+  // returns an error.
+  ErrorOr<int> StartAcceptingConnectionsLocked(
+      const std::string& service_id, int port,
+      AcceptedConnectionCallback callback)
+      ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+  bool StopAcceptingConnectionsLocked(const std::string& service_id)
+      ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+
  // Generates mDNS type.
  std::string GenerateServiceType(const std::string& service_id);

@@ -74,17 +74,16 @@ TEST_P(WifiLanTest, CanConnect) {
  CountDownLatch accept_latch(1);

  WifiLanSocket socket_for_server;
-  EXPECT_TRUE(wifi_lan_server.StartAcceptingConnections(
-      service_id, [&](const std::string& service_id, WifiLanSocket socket) {
-        socket_for_server = std::move(socket);
-        accept_latch.CountDown();
-      }));
-
  NsdServiceInfo nsd_service_info;
  nsd_service_info.SetServiceName(service_info_name);
  nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey),
                                endpoint_info_name);
-  wifi_lan_server.StartAdvertising(service_id, nsd_service_info);
+  wifi_lan_server.StartAdvertising(
+      service_id, nsd_service_info,
+      [&](const std::string& service_id, WifiLanSocket socket) {
+        socket_for_server = std::move(socket);
+        accept_latch.CountDown();
+      });

  NsdServiceInfo discovered_service_info;
  wifi_lan_client.StartDiscovery(
@@ -138,17 +137,16 @@ TEST_P(WifiLanTest, CanConnectWithMultiplex) {
  CountDownLatch accept_latch(1);

  WifiLanSocket socket_for_server;
-  EXPECT_TRUE(wifi_lan_server.StartAcceptingConnections(
-      service_id, [&](const std::string& service_id, WifiLanSocket socket) {
-        socket_for_server = std::move(socket);
-        accept_latch.CountDown();
-      }));
-
  NsdServiceInfo nsd_service_info;
  nsd_service_info.SetServiceName(service_info_name);
  nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey),
                                endpoint_info_name);
-  wifi_lan_server.StartAdvertising(service_id, nsd_service_info);
+  wifi_lan_server.StartAdvertising(
+      service_id, nsd_service_info,
+      [&](const std::string& service_id, WifiLanSocket socket) {
+        socket_for_server = std::move(socket);
+        accept_latch.CountDown();
+      });

  WifiLanSocket socket_for_client;
  SingleThreadExecutor client_executor;
@@ -204,17 +202,16 @@ TEST_P(WifiLanTest, CanCancelConnect) {
  CountDownLatch accept_latch(1);

  WifiLanSocket socket_for_server;
-  EXPECT_TRUE(wifi_lan_server.StartAcceptingConnections(
-      service_id, [&](const std::string& service_id, WifiLanSocket socket) {
-        socket_for_server = std::move(socket);
-        accept_latch.CountDown();
-      }));
-
  NsdServiceInfo nsd_service_info;
  nsd_service_info.SetServiceName(service_info_name);
  nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey),
                                endpoint_info_name);
-  wifi_lan_server.StartAdvertising(service_id, nsd_service_info);
+  wifi_lan_server.StartAdvertising(
+      service_id, nsd_service_info,
+      [&](const std::string& service_id, WifiLanSocket socket) {
+        socket_for_server = std::move(socket);
+        accept_latch.CountDown();
+      });

  NsdServiceInfo discovered_service_info;
  wifi_lan_client.StartDiscovery(
@@ -262,16 +259,15 @@ TEST_P(WifiLanTest, CanConnectWithIpAddressAndPort) {
  CountDownLatch accept_latch(1);

  WifiLanSocket socket_for_server;
-  EXPECT_TRUE(wifi_lan_server.StartAcceptingConnections(
-      service_id, [&](const std::string& service_id, WifiLanSocket socket) {
+  NsdServiceInfo nsd_service_info;
+  nsd_service_info.SetServiceName(std::string(kServiceInfoName));
+  EXPECT_TRUE(wifi_lan_server.StartAdvertising(
+      service_id, nsd_service_info,
+      [&](const std::string& service_id, WifiLanSocket socket) {
        socket_for_server = std::move(socket);
        accept_latch.CountDown();
      }));

-  NsdServiceInfo nsd_service_info;
-  nsd_service_info.SetServiceName(std::string(kServiceInfoName));
-  EXPECT_TRUE(wifi_lan_server.StartAdvertising(service_id, nsd_service_info));
-
  auto server_credentials = wifi_lan_server.GetCredentials(service_id);
  ASSERT_FALSE(server_credentials.first.empty());
  ASSERT_NE(server_credentials.second, 0);
@@ -309,13 +305,11 @@ TEST_F(WifiLanTest, CanStartAdvertising) {
  std::string service_info_name(kServiceInfoName);
  std::string endpoint_info_name(kEndpointName);

-  EXPECT_TRUE(wifi_lan_a.StartAcceptingConnections(service_id, {}));
-
  NsdServiceInfo nsd_service_info;
  nsd_service_info.SetServiceName(service_info_name);
  nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey),
                                endpoint_info_name);
-  EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id, nsd_service_info));
+  EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id, nsd_service_info, {}));
  EXPECT_TRUE(wifi_lan_a.StopAdvertising(service_id));
  env_.Stop();
 }
@@ -329,9 +323,6 @@ TEST_F(WifiLanTest, CanStartMultipleAdvertising) {
  std::string service_info_name_2("ServiceInfoName_1");
  std::string endpoint_info_name(kEndpointName);

-  EXPECT_TRUE(wifi_lan_a.StartAcceptingConnections(service_id_1, {}));
-  EXPECT_TRUE(wifi_lan_a.StartAcceptingConnections(service_id_2, {}));
-
  NsdServiceInfo nsd_service_info_1;
  nsd_service_info_1.SetServiceName(service_info_name_1);
  nsd_service_info_1.SetTxtRecord(std::string(kEndpointInfoKey),
@@ -340,8 +331,10 @@ TEST_F(WifiLanTest, CanStartMultipleAdvertising) {
  nsd_service_info_2.SetServiceName(service_info_name_2);
  nsd_service_info_2.SetTxtRecord(std::string(kEndpointInfoKey),
                                  endpoint_info_name);
-  EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id_1, nsd_service_info_1));
-  EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id_2, nsd_service_info_2));
+  EXPECT_TRUE(
+      wifi_lan_a.StartAdvertising(service_id_1, nsd_service_info_1, {}));
+  EXPECT_TRUE(
+      wifi_lan_a.StartAdvertising(service_id_2, nsd_service_info_2, {}));
  EXPECT_TRUE(wifi_lan_a.StopAdvertising(service_id_1));
  EXPECT_TRUE(wifi_lan_a.StopAdvertising(service_id_2));
  EXPECT_TRUE(wifi_lan_a.StopAcceptingConnections(service_id_1));
@@ -400,13 +393,11 @@ TEST_F(WifiLanTest, CanAdvertiseThatOtherMediumDiscover) {
                          },
                  });

-  EXPECT_TRUE(wifi_lan_a.StartAcceptingConnections(service_id, {}));
-
  NsdServiceInfo nsd_service_info;
  nsd_service_info.SetServiceName(service_info_name);
  nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey),
                                endpoint_info_name);
-  EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id, nsd_service_info));
+  EXPECT_TRUE(wifi_lan_a.StartAdvertising(service_id, nsd_service_info, {}));
  EXPECT_TRUE(discovered_latch.Await(kWaitDuration).result());
  EXPECT_TRUE(wifi_lan_a.StopAdvertising(service_id));
  EXPECT_TRUE(lost_latch.Await(kWaitDuration).result());
@@ -424,13 +415,11 @@ TEST_F(WifiLanTest, CanDiscoverThatOtherMediumAdvertise) {
  CountDownLatch discovered_latch(1);
  CountDownLatch lost_latch(1);

-  EXPECT_TRUE(wifi_lan_b.StartAcceptingConnections(service_id, {}));
-
  NsdServiceInfo nsd_service_info;
  nsd_service_info.SetServiceName(service_info_name);
  nsd_service_info.SetTxtRecord(std::string(kEndpointInfoKey),
                                endpoint_info_name);
-  wifi_lan_b.StartAdvertising(service_id, nsd_service_info);
+  wifi_lan_b.StartAdvertising(service_id, nsd_service_info, {});

  EXPECT_TRUE(wifi_lan_a.StartDiscovery(
      service_id, DiscoveredServiceCallback{
@@ -3171,31 +3171,6 @@ ErrorOr<Medium> P2pClusterPcpHandler::StartWifiLanAdvertising(
  // request comes in very quickly.
  LOG(INFO) << "P2pClusterPcpHandler::StartWifiLanAdvertising: service="
            << service_id << ": start";
-  if (!wifi_lan_medium_.IsAcceptingConnections(service_id)) {
-    ErrorOr<bool> wifi_lan_result = wifi_lan_medium_.StartAcceptingConnections(
-        service_id,
-        absl::bind_front(
-            &P2pClusterPcpHandler::WifiLanConnectionAcceptedHandler, this,
-            client, local_endpoint_id, local_endpoint_info.AsStringView(),
-            NearbyDevice::Type::kConnectionsDevice));
-    if (wifi_lan_result.has_error()) {
-      LOG(WARNING)
-          << "In StartWifiLanAdvertising("
-          << absl::BytesToHexString(local_endpoint_info.data())
-          << "), client=" << client->GetClientId()
-          << " failed to start listening for incoming WifiLan connections "
-             "to service_id="
-          << service_id;
-      return {Error(wifi_lan_result.error().operation_result_code().value())};
-    }
-    VLOG(1) << "In StartWifiLanAdvertising("
-            << absl::BytesToHexString(local_endpoint_info.data())
-            << "), client=" << client->GetClientId()
-            << " started listening for incoming WifiLan connections "
-               "to service_id = "
-            << service_id;
-  }
-
  // Generate a WifiLanServiceInfo with which to become WifiLan discoverable.
  // TODO(b/169550050): Implement UWBAddress.
  const ByteArray service_id_hash =
@@ -3220,7 +3195,6 @@ ErrorOr<Medium> P2pClusterPcpHandler::StartWifiLanAdvertising(
                 << absl::BytesToHexString(service_id_hash.data())
                 << ", endpoint_info="
                 << absl::BytesToHexString(local_endpoint_info.data()) << "}.";
-    wifi_lan_medium_.StopAcceptingConnections(service_id);
    return {
        Error(OperationResultCode::NEARBY_WIFI_LAN_ADVERTISE_TO_BYTES_FAILURE)};
  }
@@ -3231,15 +3205,18 @@ ErrorOr<Medium> P2pClusterPcpHandler::StartWifiLanAdvertising(
            << nsd_service_info.GetServiceName()
            << " with service_id=" << service_id;

-  ErrorOr<bool> wifi_lan_result =
-      wifi_lan_medium_.StartAdvertising(service_id, nsd_service_info);
+  ErrorOr<bool> wifi_lan_result = wifi_lan_medium_.StartAdvertising(
+      service_id, nsd_service_info,
+      absl::bind_front(&P2pClusterPcpHandler::WifiLanConnectionAcceptedHandler,
+                       this, client, local_endpoint_id,
+                       local_endpoint_info.AsStringView(),
+                       NearbyDevice::Type::kConnectionsDevice));
  if (wifi_lan_result.has_error()) {
-    LOG(INFO) << "In StartWifiLanAdvertising("
+    LOG(WARNING) << "In StartWifiLanAdvertising("
              << absl::BytesToHexString(local_endpoint_info.data())
              << "), client=" << client->GetClientId()
              << " couldn't advertise with WifiLanServiceInfo "
              << nsd_service_info.GetServiceName();
-    wifi_lan_medium_.StopAcceptingConnections(service_id);
    return {Error(wifi_lan_result.error().operation_result_code().value())};
  }
  VLOG(1) << "In StartWifiLanAdvertising("