-
Couldn't load subscription status.
- Fork 242
add support async fn in Server traits #593
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.
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
Conversation
| server_interior.push( | ||
| Line(fmt!(ctx, | ||
| "fn {}(&mut self, _: {}Params<{}>, _: {}Results<{}>) -> {capnp}::capability::Promise<(), {capnp}::Error> {{ {capnp}::capability::Promise::err({capnp}::Error::unimplemented(\"method {}::Server::{} not implemented\".to_string())) }}", | ||
| "fn {}(&self, _: {}Params<{}>, _: {}Results<{}>) -> impl ::std::future::Future<Output = Result<(), {capnp}::Error>> + '_ {{ async {{ Err({capnp}::Error::unimplemented(\"method {}::Server::{} not implemented\".to_string())) }} }}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all the code is single-threaded, we could've used async fn directly in the definition, making the code a tiny bit easier to read. This would require us to allow(async_fn_in_traits) though, so I decided to keep it as impl Future.
|
Wow! Thank you for putting this together. Reviewing this will be a high priority for me. |
|
It seems I forgot to check some stuff locally! My bad :( I'll fix CI tomorrow (currently kinda of late here 😅). |
|
It looks really clean! amazing work @luisholanda Love to see fewer One comment I have is on exposing Rc, RefCell etc. as part of the api; Client, ImbuedMessageBuilder. It is fairly unergonomic and we are now exposing the entire surface area of Rc as an example, small as they may be. I'd rather sacrifice a bit of performance to instead have a small wrapper abstraction (inner pattern?), that takes care of the mutability / reference counting. Strictly from an application writers perspective, I'd rather avoid having to deal with Rc, RefCell etc, if I can. It is probably a separate issue though. More generally is that we're breaking clients for certain, are there other breaking changes that should be batched in? |
What changes in this PR is that now your |
I've some proposals to improve performance and runtime integration for the They are still in the design phase, though. Overall, I would prefer to land this PR in a |
My plan was to switch the version to |
I think the improvements in this PR are important enough that we should not block them on further speculative changes. |
b266fba to
e0519f0
Compare
|
@dwrensha, I've updated the PR, fixed the CI, and applied your suggestions to the code. |
With this commit changes how we generate the `Server` traits for interfaces. We now generate `async fn` compatible traits instead of methods returning `Promise`. The methods now receive `&self` instead of `&mut self`, which causes server implementations to need `RefCell`/`Cell` for mutable state, similar to what we have for `Send` servers in other RPC systems. This is slightly different from what was discussed in capnproto#577, because we don't actually have a `Rc<impl Server>` inside `Client`, but instead `Rc<ServerDispatch>`, making the receiver a `Rc<Self>` (which would allow easier background processing spawning) would require an extra allocation, pointer chasing, and ref-counting operations. From the changes in the examples, we can see that the code is significantly easier to understand, and `pry!` is almost never needed. The only case I truly needed to use it was when the code needed to manually create `Promise` due to a `RefMut` preventing use to use `async fn` (which could cause double borrows). In matters of breaking changes, this of course breaks every pre-existing implementation, but it should be an easy migration. In the crates themselves, `capnp::capability::Server` needed to be changed to receive `Rc<Self>` instead of `&mut self`, not only the `&mut` isn't necessary anymore but also receiving a `Rc<Self>` (which we already have where it is called) also allow us to make the generated methods for `Server` traits non-'static, which makes the `async fn` syntax significantly more useful. Also, some methods in `CapabilityServerSet` which exposed the internal `Rc<RefCell<Dispatcher>>` from `Client` needed to be changed to return just `Rc<Dispatcher>`. Also, given that we're using `async/await` in the generated code, they can't be compiled in editions 2015/2018 anymore. It may be possible to support them with some added complexity in the generated code, but I don't think it is worth it. Closes: capnproto#577
|
Love the change btw, really nice work! 👏 Eager to remove all the |
e0519f0 to
54d3882
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #593 +/- ##
==========================================
- Coverage 51.64% 50.63% -1.01%
==========================================
Files 69 70 +1
Lines 33735 32319 -1416
==========================================
- Hits 17422 16366 -1056
+ Misses 16313 15953 -360 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR changes how we generate the
Servertraits for interfaces. We now generateasync fncompatible traits instead of methods returningPromise.The methods now receive
&selfinstead of&mut self, which causes server implementations to needRefCell/Cellfor mutable state, similar to what we have forSendservers in other RPC systems. This is slightly different from what was discussed in #577, because we don't actually have aRc<impl Server>insideClient, but insteadRc<ServerDispatch>, making the receiver aRc<Self>(which would allow easier background processing spawning) would require an extra allocation, pointer chasing, and ref-counting operations.From the changes in the examples, we can see that the code is significantly easier to understand, with
pry!rarely being needed. The only case I truly needed to use it was when the code needed to manually create aPromisedue to aRefMutpreventing the use ofasync fn(which could cause double borrows). For me, this is a significant improvement in UX.In matters of breaking changes, this, of course, breaks every pre-existing implementation, but it should be an easy migration. In the crates themselves,
capnp::capability::Serverneeded to be changed to receiveRc<Self>instead of&mut self, not only the&mutisn't necessary anymore but also receiving aRc<Self>(which we already have where it is called) also allow us to make the generated methods forServertraits non-'static, which makes theasync fnsyntax significantly more useful. Additionally, certain methods inCapabilityServerSet, that exposed the internalRc<RefCell<Dispatcher>>fromClient, needed to be changed to returnRc<Dispatcher>. These changes in the crates were the main reason why there isn't a switch for old/new code generation methods, as they are incompatible with each other.Also, given that we're using
async/awaitin the generated code, they can't be compiled in editions 2015/2018 anymore. It may be possible to support them with some added complexity in the generated code, but I don't think it is worth it.Closes: #577