✨ core: Support the Varlink upgrade protocol#270
Conversation
The `#[zlink::service]` and `#[proxy]` macros emit `#[cfg(feature = "std")]` around std-only generated code (FD handling and the upgrade hook). That cfg is evaluated in the crate that *uses* the macro, so any service or proxy defined in (or via) the `zlink` facade needs `zlink` itself to have a `std` feature. It didn't, so the gated code was silently dropped: upgrade handling fell back to MethodNotImplemented. Add a `std` feature and have the `tokio`/`smol` runtime features enable it. Since the facade already requires one of those runtimes via a compile_error!, std is now guaranteed on for any buildable config. Assisted-by: OpenCode (Claude Opus 4.8) Signed-off-by: Colin Walters <walters@verbum.org>
The Varlink spec lets a method set `upgrade: true`, after which the server sends a single reply and both ends hand the connection off to a custom, non-Varlink protocol (typically to speak a binary protocol for large data or pass file decriptors). Add support for this. If a server doesn't opt-in to upgrade handling, we default to returning an error. Assisted-by: OpenCode (Various models) Signed-off-by: Colin Walters <walters@verbum.org>
zeenix
left a comment
zeenix
left a comment
There was a problem hiding this comment.
Thanks so much for this PR, Colin! 👍
| tokio = ["dep:zlink-tokio", "std"] | ||
| smol = ["dep:zlink-smol", "std"] |
There was a problem hiding this comment.
That cfg is evaluated in the crate that uses the macro, so any
service or proxy defined in (or via) thezlinkfacade needszlink
itself to have astdfeature. It didn't, so the gated code was silently
dropped: upgrade handling fell back to MethodNotImplemented.
The cfg needs to be in macro code itself so the code generation itself is conditional rathe than having cfg in the generated code. Claude models want to do the latter and that is why I have this in my CLAUDE.md:
### Procedural Macros - Attribute Consumption Pattern
* `#[cfg(feature = ..)]` should go in the macro code, not the generated code.
...
to make it stop doing that. Please do the same.
| serde_json = "1.0" | ||
| tracing-subscriber = "0.3.19" | ||
| rustix = { version = "1.1.2", features = ["process"] } | ||
| rand = "0.9" |
There was a problem hiding this comment.
please split into a separate commit. Also, could you please see what else could be split so that it's easier to review this rather large change? E.g changes to each subcrate would be ideally in their own commits, especially core, proxy and service macros etc.
There was a problem hiding this comment.
E.g changes to each subcrate would be ideally in their own commits, especially core, proxy and service macros etc.
The guidelines do suggest that, but I don't quite understand the logic, we'd have to pretty artificially split things I feel? But if that's what you want, sure no problem.
There was a problem hiding this comment.
we'd have to pretty artificially split things I feel?
It's not strictly necessary that you always split for each subscrate, no. I myself don't do that, as you can see from the git log. Having said that, new low-level API addition in -core should go in a separate commit to the one using that in the -macros crate IMO. The main point was that this commit needs splitting and I think you agree. :)
| /// # Panics | ||
| /// | ||
| /// In debug builds, this will assert that any buffered writes have been flushed first. | ||
| pub fn into_parts(self) -> ConnectionParts<S> { |
There was a problem hiding this comment.
there is already split method here. If you need a struct that keeps them both, then Connection itself already does that for you and provies api to get refs to the halves.
| } | ||
|
|
||
| /// Consumes the write connection and returns the raw write half of the socket. | ||
| pub(super) fn into_inner(self) -> Write { |
There was a problem hiding this comment.
naming is a bit inconsistent with the existing write_half method. This is probably better naming though so feel free to change the name of the existing method instead (we just entered API break in the main branch) but please do put this right under that one above.
| let prefix = if self.upgrade { | ||
| "upgrade method" | ||
| } else { | ||
| "method" | ||
| }; | ||
| write!(f, "{} {}(", prefix, self.name)?; |
There was a problem hiding this comment.
I don't see any upgrade in the IDL reference. 🤔
There was a problem hiding this comment.
Thanks, I think you're right this is sloppy. Actually I hadn't meant to push this PR yet - it needs a bit more baking time. Moving this to draft.
| // on success). On a send failure, fall through to removal via | ||
| // the default `remove = true`. | ||
| let error = crate::varlink_service::Error::MethodNotImplemented { | ||
| method: alloc::borrow::Cow::Borrowed("upgrade"), |
There was a problem hiding this comment.
hint: you can just use "upgraded".into() probably? This is not macro generated code so paths don't need to be explicit.
| >; | ||
|
|
||
| /// Whether the service handles protocol upgrades. | ||
| #[cfg(feature = "std")] |
There was a problem hiding this comment.
why does upgrading requires std? Since we're in an API break already, do we really need to allow services to not support upgrading? We handle it all in service macro for them anyway and that's what they should be using? 🤔
2 participants
The Varlink spec lets a method set
upgrade: true, after which theserver sends a single reply and both ends hand the connection off to a
custom, non-Varlink protocol (typically to speak a binary protocol
for large data or pass file decriptors).
Add support for this. If a server doesn't opt-in to upgrade
handling, we default to returning an error.
Assisted-by: OpenCode (Various models)
Signed-off-by: Colin Walters walters@verbum.org