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.

Sign up for GitHub

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

Jump to bottom

use framed codecs to avoid unbounded buffer #33

Merged
merged 10 commits into from
Apr 22, 2022
Merged

use framed codecs to avoid unbounded buffer #33

merged 10 commits into from
Apr 22, 2022

Conversation

cedric05
Copy link
Contributor

@cedric05 cedric05 commented Apr 18, 2022 •
edited
Loading

fixes #32

Edit:
If approach looks good, I will look into fixing test cases

@cedric05 cedric05 mentioned this pull request Apr 18, 2022
Closed
@cedric05 cedric05 changed the title [WIP] using framed codecs to avoid unbounded buffer using framed codecs to avoid unbounded buffer Apr 18, 2022
ekzhang
ekzhang requested changes Apr 18, 2022
View reviewed changes
Copy link
Owner

@ekzhang ekzhang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the pull request! This looks good to me, I only have minor comments.

The only other issue that I notice is the interaction between Framed and proxy() after a new connection from the client. Currently stream.get_mut() turns the Framed object back into its inner TcpStream; however, this seems to be not correct if in the middle of a framed read, there are still bytes remaining in the internal buffer. I believe that these bytes will be skipped, which is a bug.

I'm basing this off the tokio::codec documentation. Not sure if there's any tests that detect this error right now, but given my lack of understanding I feel like we should either:

  1. Write a test that catches it and fix it (e.g., by calling Framed::read_buffer if the bug exists, or
  2. Explain why the bug doesn't exist.

Cargo.toml Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
src/shared.rs Outdated Show resolved Hide resolved
src/shared.rs Outdated Show resolved Hide resolved
src/shared.rs Outdated Show resolved Hide resolved
src/shared.rs Outdated Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
src/shared.rs Outdated Show resolved Hide resolved
@cedric05 cedric05 changed the title using framed codecs to avoid unbounded buffer use framed codecs to avoid unbounded buffer Apr 19, 2022
@cedric05 cedric05 force-pushed the main branch 2 times, most recently from ddf4d42 to b8558fa Compare April 19, 2022 12:06
@cedric05
Copy link
Contributor Author

cedric05 commented Apr 19, 2022 •
edited
Loading

  1. Write a test that catches it and fix it (e.g., by calling Framed::read_buffer if the bug exists, or
  2. Explain why the bug doesn't exist.
    @ekzhang
    I will have to look into this. As for rest, I resolved rest of the comments.

@cedric05 cedric05 requested a review from ekzhang April 21, 2022 04:20
@ekzhang
Copy link
Owner

ekzhang commented Apr 22, 2022

Thank you Prasanth!

@ekzhang ekzhang merged commit 9cd43f4 into ekzhang:main Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekzhang ekzhang ekzhang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unbounded byte buffer in read_until
2 participants
@cedric05 @ekzhang