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

rust: add async reading functionality #1211

Merged
merged 25 commits into from
Sep 15, 2024
Merged

rust: add async reading functionality #1211

merged 25 commits into from
Sep 15, 2024

Conversation

james-rms
Copy link
Collaborator

@james-rms james-rms commented Aug 12, 2024

Changelog

  • rust: switches the LZ4 compression dependency from lz4_flex to lz4-rs. This moves us from using a pure-rust lz4 implementation to C bindings. I believe this is worthwhile because lz4_flex does not support LZ4 "high compression mode". The practical reason for doing so in this PR is that lz4_flex does not expose interfaces that make it easy to build an AsyncRead adapter for it, but lz4-rs does.
  • rust: Adds structs to read MCAP data asynchronously in a linear stream.

Docs

  • Check generated rust docs for review.

Description

Adds an async RecordReaderimplementation, for reading MCAP data asynchronously. This is an optional feature, named tokio. I chose this feature flag name and this module name because this functionality is tied heavily into the Tokio ecosystem. If at some point we rebuild this to be async-executor-agnostic, we can add that functionality under a new module and feature flag name.

BeforeAfter

rust/src/tokio/lz4.rs Outdated Show resolved Hide resolved
rust/src/tokio/read.rs Outdated Show resolved Hide resolved
@james-rms
Copy link
Collaborator Author

@mrkline do you have strong opinions on lz4-flex vs lz4?

Copy link

@eloff eloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs one change to a test, and the rest is optional

rust/Cargo.toml Outdated Show resolved Hide resolved
rust/tests/metadata.rs Show resolved Hide resolved
if remaining == 0 {
return std::task::Poll::Ready(Ok(()));
}
let to_fill = min(min(remaining, buf.remaining()), max_read_len);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this is it fills the buffer in one shot. So the read loop doesn't ever run more than once. It would be a better test if it read e.g. a byte at a time into the buffer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it - the reader doesn't fill the buffer in one shot, it only yields up to max_read_len at a time. Is there something i'm missing?

Copy link

@eloff eloff Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it reads up to max_read_len bytes, which in the tests would usually fit in the buffer perfectly, so it exits the read loop without trying to read more (reads all the data on one pass) so if there was an issue with the subsequent iterations of the read loop, as would be exercised in the real world, then you wouldn't see it with this test.

It would be a better test if this required at least two read calls to get all the data, then the caller would have to loop twice at least and exercise that code.

Speaking from long experience of having had a bug exactly like this that wasn't caught by the tests.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to approve the PR so this doesn't hold up your work, it's up to you if you want to improve on the test or not

rust/src/tokio/read.rs Outdated Show resolved Hide resolved
rust/src/tokio/read.rs Show resolved Hide resolved
rust/src/tokio/lz4.rs Show resolved Hide resolved
rust/src/tokio/read.rs Outdated Show resolved Hide resolved
@james-rms james-rms merged commit c67c632 into main Sep 15, 2024
23 checks passed
@james-rms james-rms deleted the jrms/rust-async branch September 15, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants