Skip to content

Navigation Menu

Sign in
Appearance settings
Sign in
Sign up
Appearance settings

Commit cd7162f

Browse filesBrowse the repository at this point in the historyBrowse files
ryanofskyryanofsky
committed
Merge bitcoin-core/libmultiprocess#304: proxy: fix BuildList to use non-const iteration for interface types
d6f8588 proxy: fix BuildList to use non-const iteration for interface types (Ryan Ofsky) Pull request description: Needed for bitcoin#10102 since bitcoin#277 was merged. Since bitcoin#277, it is no longer possible to return `vector<unique_ptr<ExternalSigner>>` from `Node::listExternalSigners()` in bitcoin#10102, because the proxy server objects libmultiprocess creates to wrap the `ExternalSigner` objects need to take ownership of the objects to keep them alive, so they need to be moved out of the returned vector, which can only be done if `BuildField` is passed a non-const vector element. This PR restores previous behavior before bitcoin#277 making it possible to mutate container elements while serializing them, in cases like this where it is necessary. ACKs for top commit: ViniciusCestarii: tACK d6f8588 Tree-SHA512: 37bf03f3a3ee2b239480af53e05e4e46857db45ae8c489bda32e004d99aac3f5fa38f7fd6dba6123a0cafe3f64042ccbc1966448139d488b691d7ca16a1e8b9c
2 parents 390b5f9 + d6f8588 commit cd7162f
Copy full SHA for cd7162f

7 files changed

+66-3Lines changed: 66 additions & 3 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎include/mp/proxy-types.h‎

Copy file name to clipboardExpand all lines: include/mp/proxy-types.h
+22-3Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -285,9 +285,28 @@ void BuildList(TypeList<LocalType>, InvokeContext& invoke_context, Output&& outp
285285
{
286286
auto list = output.init(value.size());
287287
size_t i = 0;
288-
for (const auto& elem : value) {
289-
BuildField(TypeList<LocalType>(), invoke_context, ListOutput<typename decltype(list)::Builds>(list, i), elem);
290-
++i;
288+
// Iterate with an explicit iterator rather than a range-for loop so the
289+
// value category of `*it` is passed through to BuildField unchanged. This
290+
// matters for two reasons:
291+
//
292+
// - Elements must be passed as non-const so that BuildField can move out of
293+
// them, e.g. calling unique_ptr::release() to transfer ownership of an
294+
// interface pointer to the capnp server.
295+
//
296+
// - For proxy containers like std::vector<bool>, `*it` is a prvalue proxy
297+
// object rather than a reference. A range-for loop would bind it to a
298+
// named variable and demote it to an lvalue; passing `*it` directly
299+
// preserves the prvalue-ness.
300+
//
301+
// Only move out of elements when `value` itself is an rvalue container that
302+
// is about to be destroyed. When it is an lvalue reference owned by the
303+
// caller, pass elements as lvalues so BuildField does not move from them.
304+
for (auto it = value.begin(); it != value.end(); ++it, ++i) {
305+
if constexpr (std::is_lvalue_reference_v<Value&&>) {
306+
BuildField(TypeList<LocalType>(), invoke_context, ListOutput<typename decltype(list)::Builds>(list, i), *it);
307+
} else {
308+
BuildField(TypeList<LocalType>(), invoke_context, ListOutput<typename decltype(list)::Builds>(list, i), std::move(*it));
309+
}
291310
}
292311
}
293312

Collapse file

‎include/mp/type-map.h‎

Copy file name to clipboardExpand all lines: include/mp/type-map.h
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
#include <mp/type-pair.h>
1010
#include <mp/util.h>
1111

12+
#include <map>
13+
1214
namespace mp {
1315
template <typename KeyLocalType, typename ValueLocalType, typename Value, typename Output>
1416
void CustomBuildField(TypeList<std::map<KeyLocalType, ValueLocalType>>,
Collapse file

‎src/mp/gen.cpp‎

Copy file name to clipboardExpand all lines: src/mp/gen.cpp
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,7 @@ static void Generate(kj::StringPtr src_prefix,
329329
cpp_client << "#include <kj/common.h>\n";
330330
cpp_client << "#include <map>\n";
331331
cpp_client << "#include <mp/proxy.h>\n";
332+
cpp_client << "#include <mp/proxy-io.h>\n";
332333
cpp_client << "#include <mp/util.h>\n";
333334
cpp_client << "#include <string>\n";
334335
cpp_client << "#include <vector>\n";
Collapse file

‎test/mp/test/foo-types.h‎

Copy file name to clipboardExpand all lines: test/mp/test/foo-types.h
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ struct ExtendedCallback; // IWYU pragma: export
3939
struct FooCallback; // IWYU pragma: export
4040
struct FooFn; // IWYU pragma: export
4141
struct FooInterface; // IWYU pragma: export
42+
struct BarInterface; // IWYU pragma: export
4243
} // namespace messages
4344

4445
template <typename Output>
Collapse file

‎test/mp/test/foo.capnp‎

Copy file name to clipboardExpand all lines: test/mp/test/foo.capnp
+6Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") {
3737
callFnAsync @18 (context :Proxy.Context) -> ();
3838
callIntFnAsync @21 (context :Proxy.Context, arg :Int32) -> (result :Int32);
3939
passDataPointers @22 (arg :List(Data)) -> (result :List(Data));
40+
listBars @25 (context :Proxy.Context, n :Int32) -> (result :List(BarInterface));
4041
}
4142

4243
interface FooInit $Proxy.wrap("mp::test::FooInit") {
@@ -52,6 +53,11 @@ interface ExtendedCallback extends(FooCallback) $Proxy.wrap("mp::test::ExtendedC
5253
callExtended @0 (context :Proxy.Context, arg :Int32) -> (result :Int32);
5354
}
5455

56+
interface BarInterface $Proxy.wrap("mp::test::Bar") {
57+
destroy @0 (context :Proxy.Context) -> ();
58+
value @1 (context :Proxy.Context) -> (result :Int32);
59+
}
60+
5561
interface FooFn $Proxy.wrap("ProxyCallback<std::function<int()>>") {
5662
destroy @0 (context :Proxy.Context) -> ();
5763
call @1 (context :Proxy.Context) -> (result :Int32);
Collapse file

‎test/mp/test/foo.h‎

Copy file name to clipboardExpand all lines: test/mp/test/foo.h
+24Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,23 @@ class FooInit
7171
{
7272
};
7373

74+
//! A second, arbitrary interface used to test returning a
75+
//! list of interface objects.
76+
class Bar
77+
{
78+
public:
79+
virtual ~Bar() = default;
80+
virtual int value() = 0;
81+
};
82+
83+
//! Concrete Bar that returns a fixed value, used by listBars tests.
84+
class SimpleBar : public Bar
85+
{
86+
public:
87+
explicit SimpleBar(int value) : m_value(value) {}
88+
int value() override { return m_value; }
89+
int m_value;
90+
};
7491
class FooImplementation
7592
{
7693
public:
@@ -96,6 +113,13 @@ class FooImplementation
96113
double passDouble(double value) { return value; }
97114
int passFn(std::function<int()> fn) { return fn(); }
98115
std::vector<FooDataRef> passDataPointers(std::vector<FooDataRef> values) { return values; }
116+
std::vector<std::unique_ptr<Bar>> listBars(int n)
117+
{
118+
std::vector<std::unique_ptr<Bar>> result;
119+
result.reserve(n);
120+
for (int i = 0; i < n; ++i) result.push_back(std::make_unique<SimpleBar>(i));
121+
return result;
122+
}
99123
std::shared_ptr<FooCallback> m_callback;
100124
void callFn() { assert(m_fn); m_fn(); }
101125
void callFnAsync() { assert(m_fn); m_fn(); }
Collapse file

‎test/mp/test/test.cpp‎

Copy file name to clipboardExpand all lines: test/mp/test/test.cpp
+10Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,16 @@ KJ_TEST("Call FooInterface methods")
272272
KJ_REQUIRE(data_out[0] != nullptr);
273273
KJ_EXPECT(*data_out[0] == *data_in[0]);
274274
KJ_EXPECT(!data_out[1]);
275+
276+
// Test returning vector<unique_ptr<interface>> from server. This exercises
277+
// BuildList with interface element types, which requires non-const iteration
278+
// so unique_ptr::release() can transfer ownership to the proxy server.
279+
std::vector<std::unique_ptr<Bar>> bars{foo->listBars(3)};
280+
KJ_REQUIRE(bars.size() == 3u);
281+
for (int i = 0; i < 3; ++i) {
282+
KJ_REQUIRE(bars[i] != nullptr);
283+
KJ_EXPECT(bars[i]->value() == i);
284+
}
275285
}
276286

277287
KJ_TEST("Call IPC method after client connection is closed")

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.