doc: Fix and expand design.md#278
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review 6314aa1. Nice catches calling out two special cases that don't match the more general descriptions in the design doc.
I think the changes should be edited a little more for clarify, though, so left some suggestions below to consider
| ## Core Architecture | ||
|
|
||
| The `ProxyClient` and `ProxyServer` generated classes are not directly exposed to the user, as described in [usage.md](usage.md). Instead, they wrap C++ interfaces and appear to the user as pointers to an interface. They are first instantiated when calling `ConnectStream` and `ServeStream` respectively for creating the `InitInterface`. These methods establish connections through sockets, internally creating `Connection` objects wrapping a `capnp::RpcSystem` configured for client and server mode respectively. | ||
| The `ProxyServer` generated class is not directly exposed to the user. The `ProxyClient` generated class is exposed and made to be used directly, as described in [usage.md](usage.md), and inherits from the C++ interface class so it can be used as a pointer to it. They are first instantiated when calling `ConnectStream` and `ServeStream` respectively for creating the `InitInterface`. These methods establish connections through sockets, internally creating `Connection` objects wrapping a `capnp::RpcSystem` configured for client and server mode respectively. |
There was a problem hiding this comment.
In commit "doc: Fix and expand design.md" (6314aa1)
Nice catch. Current text is inaccurate in claiming the ProxyClient type is not exposed at all, because the pointer returned to the initial remote object returned by ConnectStream is a ProxyClient<InitInterface> object. But this object isn't really meant to do anything other than inherit from InitInterface and implement its methods. So I think potentially ConnectStream could be changed to return an InitInterface pointer instead of a ProxyClient<InitInterface> and it wouldn't affect anything.
All other remote objects obtained by calling InitInterface methods will be ProxyClient<Foo> objects, but only exposed to user code as Foo pointers, so that aligns with current documentation. Current documentation should be mostly accurate except for the ConnectStream exception, and it is misleading to change it to say ProxyClient generated class meant to be is exposed and used more directly.
LLM suggests following which I think would be more accurate:
The
ProxyServergenerated class is not directly exposed to the user. TheProxyClientgenerated class inherits from the C++ interface class, so user code interacts with it through the abstract interface type — theProxyClienttype itself is generally not visible or accessible without a cast.ConnectStreamreturns aunique_ptr<ProxyClient<InitInterface>>as an exception for the root Init interface, but even there users typically treat it as a pointer to the abstractInitInterface. For all interfaces returned by Init methods (e.g.,Printer,Calculator), the return type is the abstract class pointer, hiding the underlyingProxyCliententirely.
There was a problem hiding this comment.
I agree, I wanted to make it not misleading but added another misleading
Fixed on (used your suggestion with a tweak: replaced the em dash with ", and"): 18db0ab
|
|
||
| `ServerCall` uses the generated `ProxyMethod<MethodParams>::impl` pointer-to-member to invoke the actual C++ method on the wrapped implementation object. | ||
|
|
||
| A capnp interface can also declare a special `destroy` method, handled by `ServerDestroy` instead of `ServerCall`. Rather than dispatching to a C++ interface method, `ServerDestroy` calls `invokeDestroy()` on the `ProxyServer`, which resets `m_impl` and runs any registered cleanup functions, giving the client a way to synchronously destroy the wrapped object on the server side. |
There was a problem hiding this comment.
In commit "doc: Fix and expand design.md" (6314aa1)
This is worth mentioning, but placement is awkward since it is interrupting the description of the flow for server method calls by describing a special case before the general case is described and shown in the diagram.
Would suggest moving the new paragraph after the mermaid diagram and tweaking text to connect it to previously described flow. LLM suggests following which I think is an improvement:
Destroy methods are a special case: a capnp interface can declare a
destroymethod that is handled byServerDestroyinstead ofServerCall. Rather than dispatching through theServerField/ServerRet/ServerCallchain to a C++ interface method,ServerDestroycallsinvokeDestroy()on theProxyServer, which resetsm_impland runs any registered cleanup functions, giving the client a way to synchronously destroy the wrapped object on the server side.
ViniciusCestarii
force-pushed
the
doc-design-md-corrections
branch
from
6314aa1 to
18db0ab
Compare
|
Forced-push 18db0ab to add suggested changes |
| ## Core Architecture | ||
|
|
||
| The `ProxyClient` and `ProxyServer` generated classes are not directly exposed to the user, as described in [usage.md](usage.md). Instead, they wrap C++ interfaces and appear to the user as pointers to an interface. They are first instantiated when calling `ConnectStream` and `ServeStream` respectively for creating the `InitInterface`. These methods establish connections through sockets, internally creating `Connection` objects wrapping a `capnp::RpcSystem` configured for client and server mode respectively. | ||
| The `ProxyServer` generated class is not directly exposed to the user. The `ProxyClient` generated class inherits from the C++ interface class, so user code interacts with it through the abstract interface type, and the `ProxyClient` type itself is generally not visible or accessible without a cast. `ConnectStream` returns a `unique_ptr<ProxyClient<InitInterface>>` as an exception for the root Init interface, but even there users typically treat it as a pointer to the abstract `InitInterface`. For all interfaces returned by Init methods (e.g., `Printer`, `Calculator`), the return type is the abstract class pointer, hiding the underlying `ProxyClient` entirely. They are first instantiated when calling `ConnectStream` and `ServeStream` respectively for creating the `InitInterface`. These methods establish connections through sockets, internally creating `Connection` objects wrapping a `capnp::RpcSystem` configured for client and server mode respectively. |
There was a problem hiding this comment.
In commit "doc: Fix and expand design.md" (18db0ab)
This all looks accurate now. But I feel like it is also more confusing. The point this is trying to make is that library users don't really interact with the ProxyClient and ProxyClient classes directly. Library users define virtual interfaces and call interface methods that ProxyClient classes implement. And they provide interface pointers that ProxyServer classes wrap. They can create a ProxyClient object accessing an interface by calling ConnectStream, and they can create a ProxyServer object wrapping an interface by calling ConnectStream. The interface type is passed to ConnectStream and ServeStream as a template parameter, and Connect/Serve functions are typically only called once with an initial interface or InitInterface type. From there, InitInterface methods can return pointers new interfaces, causing new ProxyClient and ProxyServer objects to be created when they are called, and those interface and create and pass around pointers to other interface.
I think maybe it might make sense to structure the paragraph more like that description but I don't have a clear suggestion.
This change is probably better than the status quo since it is more technically accurate though, and it seems fine to follow up on improving this later.
There was a problem hiding this comment.
I agree. This revised version is more accurate, but it may also be harder to follow. Thanks for the feedback
8412fcdc65 Merge bitcoin-core/libmultiprocess#295: Mark Waiter m_cv as guarded by m_mutex 1593ee2d18 Merge bitcoin-core/libmultiprocess#294: test: Add passDouble smoke test 9885d7dd33 Merge bitcoin-core/libmultiprocess#286: proxy-client: fix TSan data race in clientDestroy fa35501c4f Mark Waiter m_cv as guarded by m_mutex faaedb11f8 test: Add passDouble smoke test 733c64318d Merge bitcoin-core/libmultiprocess#292: type-number: fix clang-tidy modernize-use-nullptr 9cc3479ab3 Merge bitcoin-core/libmultiprocess#291: cmake: Add `mp_headers` custom target 201abd9e3a Merge bitcoin-core/libmultiprocess#289: cmake: make target_capnp_sources use CURRENT dirs 99820c8aec Merge bitcoin-core/libmultiprocess#279: doc: Add comments to FIELD_* constants in proxy.h 73b985540c Merge bitcoin-core/libmultiprocess#278: doc: Fix and expand design.md e7e91b2e23 Merge bitcoin-core/libmultiprocess#277: Add std::unordered_set support and a helper BuildList to dedup list build handlers 91a951f59a tidy fix: modernize-use-nullptr 16362f42d0 cmake: Add `mp_headers` custom target 615a94fe3a cmake: document ONLY_CAPNP option in target_capnp_sources 90982f75c6 mpgen: iwyu changes required by previous commit 25bb3e67f3 proxy-client: fix TSan data race in clientDestroy 620f297f31 cmake: make target_capnp_sources use CURRENT dirs 9de4b885aa test: use camelCase + $Proxy.name for FooStruct fields 011b91793d type: add std::unordered_set support 20d19b9644 proxy: add BuildList helper and dedup map/set/vector build handlers e863c6cdf6 doc: Add comments to FIELD_* constants in proxy.h 18db0ab957 doc: Fix and expand design.md 61de697536 Merge bitcoin-core/libmultiprocess#273: proxy-client: tolerate exceptions from remote destroy during cleanup 9cec9d6ca5 Merge bitcoin-core/libmultiprocess#243: mpgen: support primitive std::optional struct fields 4aaff11374 Merge bitcoin-core/libmultiprocess#238: cmake, ci: updates for recent nixpkgs 2ac55a56b5 Merge bitcoin-core/libmultiprocess#218: Better error and log messages 6de92e1c73 proxy-client: tolerate exceptions from remote destroy during cleanup 90be8354d4 test: regression for ~ProxyClient destroy after peer disconnect 3c69d125a1 Merge bitcoin-core/libmultiprocess#260: event loop: tolerate unexpected exceptions in `post()` callbacks b8a48c65e6 event loop: tolerate unexpected exceptions in `post()` callbacks f787863d2c Merge bitcoin-core/libmultiprocess#270: doc: Bump version 10 > 11 a22f602910 doc: Bump version 10 > 11 4eae445d6d debug: Add TypeName() function and log statements for Proxy objects being created and destroyed f326c5b1b7 logging: Add better logging on IPC server-side failures 6dbfa56a04 mpgen: support primitive std::optional struct fields 8d1277deb5 mpgen refactor: add AccessorType function db716bbcba mpgen refactor: Move field handling code to FieldList class db7acb3ce2 ci: Fix shell.nix compatibility with CMake 4.0 91a7759a9a cmake: Fix IWYU in nix by adding CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 8412fcdc659e1379f9b4dea896c26bc1c5f3afa8
3 participants
Improve two points on design.md: