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

No option to choose buffer capacity for copy_bidrectional #6454

Closed
dd-dreams opened this issue Apr 2, 2024 · 12 comments · Fixed by #6500
Closed

No option to choose buffer capacity for copy_bidrectional #6454

dd-dreams opened this issue Apr 2, 2024 · 12 comments · Fixed by #6500
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-io Module: tokio/io

Comments

@dd-dreams
Copy link

Is your feature request related to a problem? Please describe.
io::copy_bidirectional uses it's own sizes (8KB) without caring about the reader/writer buffer capacities.

Describe the solution you'd like
A new function like io::copy_buf only for bidirectional writes/reads is requested if possible.

Describe alternatives you've considered
None.

Additional context
None.
@dd-dreams dd-dreams added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Apr 2, 2024
@mox692 mox692 added the M-io Module: tokio/io label Apr 2, 2024
@RRRadicalEdward
Copy link
Contributor

RRRadicalEdward commented Apr 12, 2024 •
edited

What about a new copy_bidirectional_with_max_size that would accept the buffer size as the last argument? Or maybe, would it be better to accept two buffer sizes for reader and writer buffers. We could also change copy_bidirectional directly, but it would be a breaking change.

I don't think it's possible to have a new function like io::copy_buf because in the case of copy_bidirectional we need to set the buffer size for writer stream as well and there isn't tokio::io::AsyncBufWrite.

@RRRadicalEdward
Copy link
Contributor

It seems like some time ago people agreed that we could just make another function that would take the size but the naming is the issue: #3572 (comment)

@RRRadicalEdward
Copy link
Contributor

There is File::set_max_buf_size. We can take it as a reference and name the new function like copy_bidirectional_with_max_buf_size.

@dd-dreams
Copy link
Author

What about a new copy_bidirectional_with_max_size that would accept the buffer size as the last argument?

This would be even better since it will allow raw FD instead of BufReader.

@dd-dreams
Copy link
Author

I think a better name for the function is copy_bidirectional_sized.

@RRRadicalEdward
Copy link
Contributor

I think a better name for the function is copy_bidirectional_sized.

No really sure on this one because, _sized postfix isn't used in contexts of custom buffer sizes.

It's hard to say what best name for it is. I thought also about copy_bidirectional_with_capacity, similar to BufReader::with_capacity.

@Darksonn
Copy link
Contributor

Yeah, I don't think _sized works. As for _with_capacity, it isn't really great either. That naming is usually used for collections, which this is not.

RRRadicalEdward added a commit to RRRadicalEdward/devolutions-gateway that referenced this issue Apr 15, 2024
@RRRadicalEdward
fix: Use 1024 buffer size for ARD VNC session
206fecd 
_ARD_ uses _MVS_ video codec which doesn't like buffering, and we need to have the buffer as minimal as possible.

Also, this commit adds new `copy_bidirectional` transport that is forked [the one from tokio](https://docs.rs/tokio/latest/tokio/io/fn.copy_bidirectional.html).
It's forked because the original function doesn't allow overriding the buffer size (8K is used by default). There is [an issue](tokio-rs/tokio#6454) on tokio side for it. We will be able to replace our fork with the upstream easily when it's ready.

Releated to Devolutions/IronVNC#338.
@RRRadicalEdward RRRadicalEdward mentioned this issue Apr 15, 2024
Closed
@dd-dreams
Copy link
Author

Maybe copy_bidirectional_buf? Similar to copy_buf.

@RRRadicalEdward
Copy link
Contributor

Maybe copy_bidirectional_buf? Similar to copy_buf.

It might not be the best solution since we wouldn't pass a buf, but the internal buf size

@dd-dreams
Copy link
Author

I guess something similar to your original suggestion likecopy_bidirectional_with_size would be feasible.

@RRRadicalEdward
Copy link
Contributor

@Darksonn Does copy_bidirectional_with_size sounds good to you?

@Darksonn
Copy link
Contributor

Yes.

RRRadicalEdward added a commit to RRRadicalEdward/tokio that referenced this issue Apr 20, 2024
@RRRadicalEdward
io: add copy_bidirectional_with_size
776f21b 
This is the same as `copy_bidirectional` but accepts the `a` -> `b` and `b` -> `a` buffers sizes.
`copy_bidirectional` uses 8Kb buffers under the hood which isn't
configurable. `copy_bidirectional_with_size` fixes it by allowing an
user to set its own sizes for underlying buffers.

Fixes: tokio-rs#6454.
@RRRadicalEdward RRRadicalEdward mentioned this issue Apr 20, 2024
Merged
RRRadicalEdward added a commit to RRRadicalEdward/tokio that referenced this issue Apr 20, 2024
@RRRadicalEdward
io: add copy_bidirectional_with_size
e5a6b01 
This is the same as `copy_bidirectional` but accepts the `a` -> `b` and `b` -> `a` buffers sizes.
`copy_bidirectional` uses 8Kb buffers under the hood which isn't
configurable. `copy_bidirectional_with_size` fixes it by allowing an
user to set its own sizes for underlying buffers.

Fixes: tokio-rs#6454.
Refs: tokio-rs#3572.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-io Module: tokio/io
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

io: add copy_bidirectional_with_sizes RRRadicalEdward/tokio
4 participants
@Darksonn @mox692 @RRRadicalEdward @dd-dreams