Skip to content

Conversation

@wyatt-herkamp
Copy link

@wyatt-herkamp wyatt-herkamp commented Jun 6, 2022

  1. Adds support for Tokio AsyncRead and AsyncWrite into RMP
  2. Makes the Sync Code a feature. (enabled by default)
  3. Throws the sync code into its own module. Everything is re exported for code compatibility

Remarks

  1. RMPV Does not implement async. I personally do not need it. So I did not implement it
  2. Tests have not been added yet.

Closes #290 and #226

@wyatt-herkamp wyatt-herkamp marked this pull request as draft June 6, 2022 19:02
@wyatt-herkamp wyatt-herkamp marked this pull request as ready for review June 8, 2022 20:30
@wyatt-herkamp wyatt-herkamp changed the title [WIP] Tokio Async Support Async Support[Tokio V1] Jun 8, 2022
@kornelski
Copy link
Collaborator

If the goal is to support AsyncRead, why not make a wrapper that converts AsyncRead to io::Read, and keeps rest of the code the same?

@wyatt-herkamp
Copy link
Author

If the goal is to support AsyncRead, why not make a wrapper that converts AsyncRead to io::Read, and keeps rest of the code the same?

I think providing support for async is a better option than using Tokio block or wrapping the types.

What I am using this library for I originally just read the data and then passed it into the sync version. However, I thought implementing would be worth it because I am going to be using this library a lot in my code. Also with other people wanting the feature why not

@kornelski
Copy link
Collaborator

kornelski commented Jun 9, 2022

I know AsyncRead/Write support has been a frequent ask, so there's a bunch of people who want this functionality. However, I think the ask is more of a "XY problem", and this library does not need to support AsyncRead/Write so deeply internally in order to work well with tokio and AsyncRead/Write streams. An Async <> std::io adapter would work great and achieve the same thing with fraction of the complexity.

This library doesn't have an API that allows accessing a partially-parsed data, so no matter how you read the data — whether async stream byte by byte or buffer it all to a Vec first, you won't get it any sooner. Buffering whole response and then parsing is for most typical message sizes indistinguishable from parsing from an async stream. Reading from an Async<>std::io adapter can do streaming parsing and have as low if not lower memory overhead than .await-based implementation. The sync implementation can be faster and more efficient, because sync code reading from a byte buffer will optimize much better than futures that may poll and suspend on every byte. I'm concerned that such widespread use of .await in the parser will generate a lot of code bloat and large Future objects.

So as far as I can tell this adds no really new functionality to the library (like streaming data and using partial unbuffered results), apart of small convenience of not having to use Async<>std::io adapter. If you implemented such adapter as part of this library, it would support the same AsyncRead/Write with a fraction of the code.

I appreciate you're working on this library. I regret being a naysayer, but from my part-time maintainer perspective large code changes and doubling of public API is a burden for me to maintain later.

@wyatt-herkamp
Copy link
Author

That's fine.

I saw your responses to the original issues however what I ended up doing to cope with the lack of a Async Read/Write support I ended up starting to implement my own message pack lib to cope with it. So I thought instead of making a bunch of code for my project. Push the code I am going to write unto the main project.

I will keep my fork open for anyone else who wants it. It is under MIT for a reason. I was wanting this feature for specially dealing with the binary heads for my application.

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

Labels

None yet

2 participants