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

Use BufResult #1165

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Use BufResult #1165

wants to merge 1 commit into from

Conversation

a10y
Copy link
Contributor

@a10y a10y commented Oct 30, 2024

Currently our async IO traits have methods that receive an owned buffer (in the form of BytesMut) and return an io::Result<BytesMut> which gives the buffer back at the end.

The trouble is, in the case of an error the buffer will leak. This is not a problem yet since we currently allocate a fresh buffer for every IO operation, but as we move toward buffer pools and completion-based IO operations, we don't want to leak these buffers when IO errors occur.

This PR makes a new BufResult<T> which is just a (io::Result<T>, BytesMut) to ensure that you always return ownership of the buffer to the caller even in the event of IO failures.

Currently this doesn't change any behavior, but when we swtich to pre-allocated buffers and IO operations using those, it will matter.

@a10y a10y marked this pull request as ready for review October 30, 2024 18:44
@a10y
Copy link
Contributor Author

a10y commented Oct 30, 2024

I'll put this back to draft so it can be sequenced after #1124

@a10y a10y marked this pull request as draft October 30, 2024 18:47
@a10y
Copy link
Contributor Author

a10y commented Oct 30, 2024

Note to self: double-check compio's approach and see if that's better: https://github.com/compio-rs/compio/blob/master/compio-buf/src/buf_result.rs

@robert3005
Copy link
Member

the more code like that I write the more I prefer an explicit struct instead of anonymous tuple

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

Successfully merging this pull request may close these issues.

2 participants