Skip to content

proxy: fix BuildList to use non-const iteration for interface types #304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ryanofsky wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

proxy: fix BuildList to use non-const iteration for interface types #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/mp/proxy-types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ void BuildList(TypeList<LocalType>, InvokeContext& invoke_context, Output&& outp
{
auto list = output.init(value.size());
size_t i = 0;
for (const auto& elem : value) {
for (auto&& elem : value) {

@ryanofsky ryanofsky Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "proxy: fix BuildList to use non-const iteration for interface types" (a0b01d7)

Note, while this change mostly restores behavior to what it was before #277, passing elem to BuildField below as an lvalue reference regardless of whether the container is lvalue or rvalue, it might make more sense to pass elem as an rvalue if the container is an rvalue, or as an lvalue if it's an lvalue with something like:

for (auto&& elem : value) {
    if constexpr (std::is_lvalue_reference_v<Value&&>) {
        BuildField(/*...*/, elem);
    } else {
        BuildField(/*...*/, std::move(elem));
    }
}

This could be safer because it could allow BuildField to distinguish based on the way the container is being used and only move from temporary containers that are about to be destroyed, not containers passed as references.

Also it might be better to replace for (auto&& elem : value) with for (auto it = elem.begin(); it != elem.end(); ++it) like the previous code had since this would could also preserve rvaluedness of dereferencing the iterator, if the iterator deferences as a proxy object instead of a reference like vector`

Which is just to say that a0b01d7 is a minimal change restoring ability to return lists of interface pointers, but it might make sense to actually increase safety here as well.

BuildField(TypeList<LocalType>(), invoke_context, ListOutput<typename decltype(list)::Builds>(list, i), elem);
++i;
}
Expand Down
1 change: 1 addition & 0 deletions src/mp/gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ static void Generate(kj::StringPtr src_prefix,
cpp_client << "#include <kj/common.h>\n";
cpp_client << "#include <map>\n";
cpp_client << "#include <mp/proxy.h>\n";
cpp_client << "#include <mp/proxy-io.h>\n";
cpp_client << "#include <mp/util.h>\n";
cpp_client << "#include <string>\n";
cpp_client << "#include <vector>\n";
Expand Down
1 change: 1 addition & 0 deletions test/mp/test/foo.capnp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") {
callFnAsync @18 (context :Proxy.Context) -> ();
callIntFnAsync @21 (context :Proxy.Context, arg :Int32) -> (result :Int32);
passDataPointers @22 (arg :List(Data)) -> (result :List(Data));
listCallbacks @24 (context :Proxy.Context, n :Int32) -> (result :List(FooCallback));
}

interface FooCallback $Proxy.wrap("mp::test::FooCallback") {
Expand Down
16 changes: 16 additions & 0 deletions test/mp/test/foo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ class FooCallback
virtual int call(int arg) = 0;
};

//! Concrete FooCallback that returns a fixed value, used by listCallbacks tests.
class SimpleCallback : public FooCallback
{
public:
explicit SimpleCallback(int value) : m_value(value) {}
int call(int) override { return m_value; }
int m_value;
};

class ExtendedCallback : public FooCallback
{
public:
Expand Down Expand Up @@ -89,6 +98,13 @@ class FooImplementation
double passDouble(double value) { return value; }
int passFn(std::function<int()> fn) { return fn(); }
std::vector<FooDataRef> passDataPointers(std::vector<FooDataRef> values) { return values; }
std::vector<std::unique_ptr<FooCallback>> listCallbacks(int n)
{
std::vector<std::unique_ptr<FooCallback>> result;
result.reserve(n);
for (int i = 0; i < n; ++i) result.push_back(std::make_unique<SimpleCallback>(i));
return result;
}
std::shared_ptr<FooCallback> m_callback;
void callFn() { assert(m_fn); m_fn(); }
void callFnAsync() { assert(m_fn); m_fn(); }
Expand Down
10 changes: 10 additions & 0 deletions test/mp/test/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,16 @@ KJ_TEST("Call FooInterface methods")
KJ_REQUIRE(data_out[0] != nullptr);
KJ_EXPECT(*data_out[0] == *data_in[0]);
KJ_EXPECT(!data_out[1]);

// Test returning vector<unique_ptr<interface>> from server. This exercises
// BuildList with interface element types, which requires non-const iteration
// so unique_ptr::release() can transfer ownership to the proxy server.
std::vector<std::unique_ptr<FooCallback>> callbacks{foo->listCallbacks(3)};
KJ_REQUIRE(callbacks.size() == 3u);
for (int i = 0; i < 3; ++i) {
KJ_REQUIRE(callbacks[i] != nullptr);
KJ_EXPECT(callbacks[i]->call(0) == i);

@Sjors Sjors Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this isn't really a callback flow, so the terminology had me confused about the test. Might be better to add another (Bar) interface to make it more clear.

diff --git a/test/mp/test/foo-types.h b/test/mp/test/foo-types.h
index b96eabfcfb..96506a9f50 100644
--- a/test/mp/test/foo-types.h
+++ b/test/mp/test/foo-types.h
@@ -40,4 +40,5 @@ struct FooCallback; // IWYU pragma: export
 struct FooFn; // IWYU pragma: export
 struct FooInterface; // IWYU pragma: export
+struct BarInterface; // IWYU pragma: export
 } // namespace messages

diff --git a/test/mp/test/foo.capnp b/test/mp/test/foo.capnp
index edf7841c8a..0896274209 100644
--- a/test/mp/test/foo.capnp
+++ b/test/mp/test/foo.capnp
@@ -37,5 +37,5 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") {
     callIntFnAsync @21 (context :Proxy.Context, arg :Int32) -> (result :Int32);
     passDataPointers @22 (arg :List(Data)) -> (result :List(Data));
-    listCallbacks @24 (context :Proxy.Context, n :Int32) -> (result :List(FooCallback));
+    listBars @24 (context :Proxy.Context, n :Int32) -> (result :List(BarInterface));
 }

@@ -49,4 +49,9 @@ interface ExtendedCallback extends(FooCallback) $Proxy.wrap("mp::test::ExtendedC
 }

+interface BarInterface $Proxy.wrap("mp::test::Bar") {
+    destroy @0 (context :Proxy.Context) -> ();
+    value @1 (context :Proxy.Context) -> (result :Int32);
+}
+
 interface FooFn $Proxy.wrap("ProxyCallback<std::function<int()>>") {
     destroy @0 (context :Proxy.Context) -> ();
diff --git a/test/mp/test/foo.h b/test/mp/test/foo.h
index 17aeeb8388..5c881117ce 100644
--- a/test/mp/test/foo.h
+++ b/test/mp/test/foo.h
@@ -60,13 +60,4 @@ public:
 };

-//! Concrete FooCallback that returns a fixed value, used by listCallbacks tests.
-class SimpleCallback : public FooCallback
-{
-public:
-    explicit SimpleCallback(int value) : m_value(value) {}
-    int call(int) override { return m_value; }
-    int m_value;
-};
-
 class ExtendedCallback : public FooCallback
 {
@@ -75,4 +66,27 @@ public:
 };

+//! A second, arbitrary interface used to test returning a
+//! list of interface objects.
+class Bar
+{
+public:
+    virtual ~Bar() = default;
+    virtual int value() = 0;
+};
+
+//! Concrete Bar that returns a fixed value, used by listBars tests.
+class SimpleBar : public Bar
+{
+public:
+    explicit SimpleBar(int value) : m_value(value) {}
+    int value() override { return m_value; }
+    int m_value;
+};
+
 class FooImplementation
 {
@@ -99,9 +113,9 @@ public:
     int passFn(std::function<int()> fn) { return fn(); }
     std::vector<FooDataRef> passDataPointers(std::vector<FooDataRef> values) { return values; }
-    std::vector<std::unique_ptr<FooCallback>> listCallbacks(int n)
+    std::vector<std::unique_ptr<Bar>> listBars(int n)
     {
-        std::vector<std::unique_ptr<FooCallback>> result;
+        std::vector<std::unique_ptr<Bar>> result;
         result.reserve(n);
-        for (int i = 0; i < n; ++i) result.push_back(std::make_unique<SimpleCallback>(i));
+        for (int i = 0; i < n; ++i) result.push_back(std::make_unique<SimpleBar>(i));
         return result;
     }
diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp
index 41ca506c62..41aa4d67fd 100644
--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -263,9 +263,9 @@ KJ_TEST("Call FooInterface methods")
     // BuildList with interface element types, which requires non-const iteration
     // so unique_ptr::release() can transfer ownership to the proxy server.
-    std::vector<std::unique_ptr<FooCallback>> callbacks{foo->listCallbacks(3)};
-    KJ_REQUIRE(callbacks.size() == 3u);
+    std::vector<std::unique_ptr<Bar>> bars{foo->listBars(3)};
+    KJ_REQUIRE(bars.size() == 3u);
     for (int i = 0; i < 3; ++i) {
-        KJ_REQUIRE(callbacks[i] != nullptr);
-        KJ_EXPECT(callbacks[i]->call(0) == i);
+        KJ_REQUIRE(bars[i] != nullptr);
+        KJ_EXPECT(bars[i]->value() == i);
     }
 }

@ryanofsky ryanofsky Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #304 (comment)

IIUC this isn't really a callback flow, so the terminology had me confused about the test. Might be better to add another (Bar) interface to make it more clear.

Yeah you are right. The server is returning callback objects in the sense of "you can call me back later by using these objects" not taking callback objects that it can use to call into the client later. So your example makes more sense and its better to avoid the callback term here.

}
}

KJ_TEST("Call IPC method after client connection is closed")
Expand Down
Loading