Skip to content
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

AsyncDeserializer #2739

Open
coder0xff opened this issue May 13, 2024 · 6 comments
Open

AsyncDeserializer #2739

coder0xff opened this issue May 13, 2024 · 6 comments

Comments

@coder0xff
Copy link

I'm considering extending serde to deserialize (and serialize) from a futures::io::AsyncRead (and AsyncWrite). Would it be enough to make an AsyncDeserializer, or is an AsyncVisitor also needed? I don't yet fully understand what the Visitor does, but since the method signatures take a value as input (at least for primitive types) it seems that reading from IO is already complete by the time Visitors are in play.

@oli-obk
Copy link
Member

oli-obk commented May 22, 2024

At that point you're probably better off just writing your own async_serde crate, as it will not really share anything with serde. Afaict you'll need your own Serialize and Deserialize traits, too, as everything will need to have async functions if it ever ends up invoking a Deserializer method that could perform an async operation (like reading from AsyncRead).

@coder0xff
Copy link
Author

coder0xff commented May 25, 2024

Afaict you'll need your own Serialize and Deserialize traits, too

You've pinpointed why I want to do this. I recently did exactly what you're suggesting. I made a bespoke async serialization library, derive macros and all, when I learned that serde couldn't accommodate async. When it works it works great, but it will never work for any library I don't control, because Rust doesn't permit implementing foreign traits on foreign types. I learned that, yes I can add hand-written serializers to the library where I define my trait, or fall back to blocking serde, but I shouldn't have to, and it flies in the face of separation of concerns.

I'll extend serde maintaining backward compatibility so any type already using #[derive(Serialize, Deserialize)] will generate AsyncSerialize, AsyncDeserialize, and AsyncVisitor, implementations. Then AsyncSerializer and AsyncDeserializer can be implemented by libraries like serde-rs/json so that we can all use async, just like we all enjoy today with blocking IO, thanks to serde being the one true serialization library.

P.S. I figured out that it would require an AsyncVisitor in fact, since Visitors uses Deserialize implementations in turn for composites.

@oli-obk
Copy link
Member

oli-obk commented May 25, 2024

Ah yes, the derive macros could be used to generate async impls together with the sync ones.

While it's all possible, I don't think we'll be taking on the extra maintenance burden of having two of everything (even if the async parts can be behind a cargo feature, so only ppl using it will pay the cost).

@coder0xff
Copy link
Author

coder0xff commented May 26, 2024

I'll choose to take the qualification in your second paragraph as a non-zero possibility. I wouldn't make two copies of everything of course, since there should be ample opportunity for code reuse. Primitive values are reusable as is. The composite types just need a few new trait methods in another impl. Based on my still limited perspective, the most challenging and most useful aspect of the crate is producing the data model that drives macro expansion, and that would be untouched. Generation is comparatively trivial.

I have no doubts that my pull request will be considered on its merits without preconceptions when it arrives.

@oli-obk
Copy link
Member

oli-obk commented May 26, 2024

have no doubts that my pull request will be considered on its merits without preconceptions when it arrives.

We don't have the review capacity for more than trivial features. At the current rate you're looking at a multi year wait time before you get the first maintainer review

@coder0xff
Copy link
Author

I understand. Until then, [patch.crates-io] in cargo.toml will work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants