ipcz: Propagate node disconnection to routers
Implements node disconnection propagation. When a NodeLink is severed,
it will now notify all Routers to which it has a bound sublink. Routers
treat this as equivalent to route closure from the appropriate end of
the route, if that end was not already closed.
Bug: 1299283
Change-Id: I47c1a838173e354a9421278c50ff5b5b37fdbdc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3659408
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1007148}
NOKEYCHECK=True
GitOrigin-RevId: 7908ff715a08f2f25837fb7cc05e8e504538227a
diff --git a/src/ipcz/local_router_link.cc b/src/ipcz/local_router_link.cc
index 1314d3e..3d0ce11 100644
--- a/src/ipcz/local_router_link.cc
+++ b/src/ipcz/local_router_link.cc
@@ -82,6 +82,11 @@
return state_->GetRouter(side_.opposite()).get() == &router;
}
+bool LocalRouterLink::IsRemoteLinkTo(const NodeLink& node_link,
+ SublinkId sublink) {
+ return false;
+}
+
void LocalRouterLink::AcceptParcel(Parcel& parcel) {
if (Ref<Router> receiver = state_->GetRouter(side_.opposite())) {
receiver->AcceptInboundParcel(parcel);
diff --git a/src/ipcz/local_router_link.h b/src/ipcz/local_router_link.h
index 393b451..2137609 100644
--- a/src/ipcz/local_router_link.h
+++ b/src/ipcz/local_router_link.h
@@ -26,6 +26,7 @@
// RouterLink:
LinkType GetType() const override;
bool HasLocalPeer(const Router& router) override;
+ bool IsRemoteLinkTo(const NodeLink& node_link, SublinkId sublink) override;
void AcceptParcel(Parcel& parcel) override;
void AcceptRouteClosure(SequenceNumber sequence_length) override;
void Deactivate() override;
diff --git a/src/ipcz/node.cc b/src/ipcz/node.cc
index b604b10..aa24e13 100644
--- a/src/ipcz/node.cc
+++ b/src/ipcz/node.cc
@@ -124,4 +124,29 @@
return name;
}
+// static
+void Node::SimulateDisconnectForTesting(IpczHandle first, IpczHandle second) {
+ Node* node0 = Node::FromHandle(first);
+ Node* node1 = Node::FromHandle(second);
+
+ node0->SimulateDisconnectForTesting(node1->GetAssignedName());
+ node1->SimulateDisconnectForTesting(node0->GetAssignedName());
+}
+
+void Node::SimulateDisconnectForTesting(const NodeName& name) {
+ absl::MutexLock lock(&mutex_);
+ auto it = node_links_.find(name);
+ if (it == node_links_.end()) {
+ return;
+ }
+
+ Ref<NodeLink> link = std::move(it->second);
+ node_links_.erase(it);
+
+ if (broker_link_ == link) {
+ broker_link_.reset();
+ }
+ link->SimulateDisconnectForTesting();
+}
+
} // namespace ipcz
diff --git a/src/ipcz/node.h b/src/ipcz/node.h
index 7806a56..d00f2ad 100644
--- a/src/ipcz/node.h
+++ b/src/ipcz/node.h
@@ -87,9 +87,16 @@
// randomness.
NodeName GenerateRandomName() const;
+ // Forces disconnection of two specific nodes which have an established
+ // NodeLink to each other.
+ static void SimulateDisconnectForTesting(IpczHandle first, IpczHandle second);
+
private:
~Node() override;
+ // Forces disconnection of a specific node from this one.
+ void SimulateDisconnectForTesting(const NodeName& name);
+
const Type type_;
const IpczDriver driver_;
const IpczDriverHandle driver_node_;
diff --git a/src/ipcz/node_link.cc b/src/ipcz/node_link.cc
index 2e3afda..fb301fe 100644
--- a/src/ipcz/node_link.cc
+++ b/src/ipcz/node_link.cc
@@ -138,6 +138,11 @@
transport_->Transmit(message);
}
+void NodeLink::SimulateDisconnectForTesting() {
+ OnTransportError();
+ Deactivate();
+}
+
SequenceNumber NodeLink::GenerateOutgoingSequenceNumber() {
return SequenceNumber(next_outgoing_sequence_number_generator_.fetch_add(
1, std::memory_order_relaxed));
@@ -243,7 +248,15 @@
}
void NodeLink::OnTransportError() {
- // TODO: Notify all routers attached to sublinks here that this link is dead.
+ SublinkMap sublinks;
+ {
+ absl::MutexLock lock(&mutex_);
+ sublinks.swap(sublinks_);
+ }
+
+ for (auto& [id, sublink] : sublinks) {
+ sublink.receiver->NotifyLinkDisconnected(*this, id);
+ }
}
NodeLink::Sublink::Sublink(Ref<RemoteRouterLink> router_link,
diff --git a/src/ipcz/node_link.h b/src/ipcz/node_link.h
index ed1aa12..537129f 100644
--- a/src/ipcz/node_link.h
+++ b/src/ipcz/node_link.h
@@ -105,6 +105,10 @@
// memory.
void Transmit(Message& message);
+ // Simulates disconnection by invoking the code path which handles transport
+ // errors and deactiving this link.
+ void SimulateDisconnectForTesting();
+
private:
NodeLink(Ref<Node> node,
LinkSide link_side,
diff --git a/src/ipcz/remote_router_link.cc b/src/ipcz/remote_router_link.cc
index 9bd1ce4..7e9838a 100644
--- a/src/ipcz/remote_router_link.cc
+++ b/src/ipcz/remote_router_link.cc
@@ -44,6 +44,11 @@
return false;
}
+bool RemoteRouterLink::IsRemoteLinkTo(const NodeLink& node_link,
+ SublinkId sublink) {
+ return node_link_.get() == &node_link && sublink_ == sublink;
+}
+
void RemoteRouterLink::AcceptParcel(Parcel& parcel) {
const absl::Span<Ref<APIObject>> objects = parcel.objects_view();
diff --git a/src/ipcz/remote_router_link.h b/src/ipcz/remote_router_link.h
index e2111bb..4407db7 100644
--- a/src/ipcz/remote_router_link.h
+++ b/src/ipcz/remote_router_link.h
@@ -47,6 +47,7 @@
// RouterLink:
LinkType GetType() const override;
bool HasLocalPeer(const Router& router) override;
+ bool IsRemoteLinkTo(const NodeLink& node_link, SublinkId sublink) override;
void AcceptParcel(Parcel& parcel) override;
void AcceptRouteClosure(SequenceNumber sequence_length) override;
void Deactivate() override;
diff --git a/src/ipcz/router.cc b/src/ipcz/router.cc
index 5c90b9d..97e1231 100644
--- a/src/ipcz/router.cc
+++ b/src/ipcz/router.cc
@@ -49,6 +49,9 @@
bool Router::HasLocalPeer(Router& router) {
absl::MutexLock lock(&mutex_);
+ if (!outward_link_) {
+ return false;
+ }
return outward_link_->HasLocalPeer(router);
}
@@ -159,7 +162,9 @@
return false;
}
- if (!inward_link_) {
+ if (inward_link_) {
+ inward_link_->AcceptRouteClosure(sequence_length);
+ } else {
status_.flags |= IPCZ_PORTAL_STATUS_PEER_CLOSED;
if (inbound_parcels_.IsSequenceFullyConsumed()) {
status_.flags |= IPCZ_PORTAL_STATUS_DEAD;
@@ -358,6 +363,35 @@
Flush();
}
+void Router::NotifyLinkDisconnected(const NodeLink& node_link,
+ SublinkId sublink) {
+ Ref<RouterLink> dead_outward_link;
+ SequenceNumber inbound_sequence_length;
+ Ref<RouterLink> dead_inward_link;
+ SequenceNumber outbound_sequence_length;
+ {
+ absl::MutexLock lock(&mutex_);
+ if (outward_link_ && outward_link_->IsRemoteLinkTo(node_link, sublink)) {
+ dead_outward_link = std::move(outward_link_);
+ inbound_sequence_length = inbound_parcels_.GetCurrentSequenceLength();
+ } else if (inward_link_ &&
+ inward_link_->IsRemoteLinkTo(node_link, sublink)) {
+ dead_inward_link = std::move(inward_link_);
+ outbound_sequence_length = outbound_parcels_.GetCurrentSequenceLength();
+ }
+ }
+
+ if (dead_outward_link) {
+ AcceptRouteClosureFrom(dead_outward_link->GetType(),
+ inbound_sequence_length);
+ }
+
+ if (dead_inward_link) {
+ AcceptRouteClosureFrom(dead_inward_link->GetType(),
+ outbound_sequence_length);
+ }
+}
+
void Router::Flush() {
Ref<RouterLink> outward_link;
Ref<RouterLink> inward_link;
diff --git a/src/ipcz/router.h b/src/ipcz/router.h
index 9c674bc..2a09f30 100644
--- a/src/ipcz/router.h
+++ b/src/ipcz/router.h
@@ -127,6 +127,11 @@
void BeginProxyingToNewRouter(NodeLink& to_node_link,
const RouterDescriptor& descriptor);
+ // Notifies this Router that one of its links has been disconnected from a
+ // remote node. The link is identified by a combination of a specific NodeLink
+ // and SublinkId.
+ void NotifyLinkDisconnected(const NodeLink& node_link, SublinkId sublink);
+
private:
~Router() override;
diff --git a/src/ipcz/router_link.h b/src/ipcz/router_link.h
index 84f3867..332b3ca 100644
--- a/src/ipcz/router_link.h
+++ b/src/ipcz/router_link.h
@@ -7,10 +7,12 @@
#include "ipcz/link_type.h"
#include "ipcz/sequence_number.h"
+#include "ipcz/sublink_id.h"
#include "util/ref_counted.h"
namespace ipcz {
+class NodeLink;
class Parcel;
class Router;
@@ -32,6 +34,10 @@
// Returns true iff this is a LocalRouterLink whose peer router is `router`.
virtual bool HasLocalPeer(const Router& router) = 0;
+ // Returns true iff this is a RemoteRouterLink routing over `node_link` via
+ // `sublink`.
+ virtual bool IsRemoteLinkTo(const NodeLink& node_link, SublinkId sublink) = 0;
+
// Passes a parcel to the Router on the other side of this link to be queued
// and/or router further.
virtual void AcceptParcel(Parcel& parcel) = 0;
diff --git a/src/remote_portal_test.cc b/src/remote_portal_test.cc
index 36c4bf2..d05d759 100644
--- a/src/remote_portal_test.cc
+++ b/src/remote_portal_test.cc
@@ -5,6 +5,7 @@
#include <utility>
#include "ipcz/ipcz.h"
+#include "ipcz/node.h"
#include "test/multinode_test.h"
#include "testing/gtest/include/gtest/gtest.h"
@@ -103,9 +104,49 @@
EXPECT_EQ("hi", message);
}
+ CloseAll({a, b});
VerifyEndToEnd(c, d);
- CloseAll({a, b, c, d, node1, node0});
+ CloseAll({c, d, node1, node0});
+}
+
+TEST_P(RemotePortalTest, DisconnectThroughProxy) {
+ // Exercises node disconnection. Namely if portals on nodes 1 and 3 are
+ // connected via proxy on node 2, and node 3 disappears, node 1's portal
+ // should observe peer closure.
+ IpczHandle node0 = CreateBrokerNode();
+ IpczHandle node1 = CreateNonBrokerNode();
+ IpczHandle node2 = CreateNonBrokerNode();
+ IpczHandle node3 = CreateNonBrokerNode();
+
+ auto [a, b] = ConnectBrokerToNonBroker(node0, node1);
+ auto [c, d] = ConnectBrokerToNonBroker(node0, node2);
+ auto [e, f] = ConnectBrokerToNonBroker(node0, node3);
+
+ auto [q, p] = OpenPortals(node0);
+
+ // Send `q` to `node1` and `p` to `node2`.
+ Put(a, "", {&q, 1});
+ Put(c, "", {&p, 1});
+ EXPECT_EQ(IPCZ_RESULT_OK, WaitToGet(b, nullptr, {&q, 1}));
+ EXPECT_EQ(IPCZ_RESULT_OK, WaitToGet(d, nullptr, {&p, 1}));
+
+ // Now forward 'p' back to `node0` and then again to `node3`. This ensures
+ // that node2 will proxy between node1 and node3 for at least a small window
+ // of time.
+ Put(d, "", {&p, 1});
+ EXPECT_EQ(IPCZ_RESULT_OK, WaitToGet(c, nullptr, {&p, 1}));
+ Put(e, "", {&p, 1});
+ EXPECT_EQ(IPCZ_RESULT_OK, WaitToGet(f, nullptr, {&p, 1}));
+
+ // TODO: Once proxy reduction is implemented, the test setup should wait for
+ // a direct link between node2 and node3 before then severing only that
+ // connection. Without proxy reduction, no such direct link exists yet.
+ ipcz::Node::SimulateDisconnectForTesting(node0, node3);
+
+ EXPECT_EQ(IPCZ_RESULT_OK, WaitForConditionFlags(q, IPCZ_TRAP_PEER_CLOSED));
+
+ CloseAll({a, b, c, d, e, f, q, p, node3, node2, node1, node0});
}
INSTANTIATE_MULTINODE_TEST_SUITE_P(RemotePortalTest);
