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

fix: Fix read result not full #3350

Merged
merged 1 commit into from
Oct 21, 2023
Merged

Conversation

jiaoew1991
Copy link
Contributor

After actual testing, it was found that using self.inner.read will read data based on the size of internal bytes. By using self.inner.read_exact, can read the input buffer size directly.

Signed-off-by: Enwei Jiao <enwei.jiao@zilliz.com>
Copy link
Contributor

@xyjixyjixyji xyjixyjixyji left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

bindings/c/src/types.rs Outdated Show resolved Hide resolved
@xyjixyjixyji xyjixyjixyji merged commit 1ade687 into apache:main Oct 21, 2023
31 checks passed
@Xuanwo
Copy link
Member

Xuanwo commented Oct 21, 2023

This PR is wrong, we can't guarantee that the source has enough data.

@xyjixyjixyji
Copy link
Contributor

This PR is wrong, we can't guarantee that the source has enough data.

I thought this function is supposed to read 'len' bytes?

@Xuanwo Xuanwo changed the title Fix read result not full fix: Fix read result not full Oct 21, 2023
@xyjixyjixyji
Copy link
Contributor

xyjixyjixyji commented Oct 21, 2023

This PR is wrong, we can't guarantee that the source has enough data.

/// ```Rust
/// # futures::executor::block_on(async {
/// use futures::io::{self, AsyncReadExt, Cursor};
///
/// let mut reader = Cursor::new([1, 2, 3, 4]);
/// let mut output = [0u8; 5];
///
/// let result = reader.read_exact(&mut output).await;
///
/// assert_eq!(result.unwrap_err().kind(), io::ErrorKind::UnexpectedEof);
/// # });
/// ```

This is the documentation of read_exact, which shows reader.read_exact() returns an error if the source does not have enough data.

Is there anything different between opendal_reader_read and this?

@Xuanwo
Copy link
Member

Xuanwo commented Oct 21, 2023

This is the documentation of read_exact, which shows reader.read_exact() returns an error if the source does not have enough data.

Please note that read() and read_exact() are different APIs that have different semantic.

  • For read(buf, len) -> Result<read_size>, users will input a buffer with the maximum number of bytes to read and we will return read_size by which 0 indicates we have reached EOF.
  • For read_exact(buf, len) -> Result<()>, users will input a buffer with expected number of bytes to read and we will return UnexpectedEof when source doesn't have enough data to read.

It's ok to provide an API called opendal_reader_read_exact() but we can't implement like this for opendal_reader_read(). Users must repeatedly call opendal_reader_read() until it returns 0, indicating that the buffer is full or the source has reached EOF.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 21, 2023

I thought this function is supposed to read 'len' bytes?

len represents the size of this buffer. However, there's no guarantee that we'll need to fill it completely. It's possible for underlying storage to read only 1 bytes every time.

@xyjixyjixyji
Copy link
Contributor

I thought this function is supposed to read 'len' bytes?

len represents the size of this buffer. However, there's no guarantee that we'll need to fill it completely. It's possible for underlying storage to read only 1 bytes every time.

I see, I will raise a PR to revert this.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 21, 2023

I see, I will raise a PR to revert this.

Fixed in #3282

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

Successfully merging this pull request may close these issues.

None yet

3 participants