Skip to content

fix: Fix read result not full#3350

Merged
xyjixyjixyji merged 1 commit intoapache:mainfrom
jiaoew1991:fix-read-not-full
Oct 21, 2023
Merged

fix: Fix read result not full#3350
xyjixyjixyji merged 1 commit intoapache:mainfrom
jiaoew1991:fix-read-not-full

Conversation

@jiaoew1991
Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread bindings/c/src/types.rs Outdated
@xyjixyjixyji xyjixyjixyji merged commit 1ade687 into apache:main Oct 21, 2023
@Xuanwo
Copy link
Copy Markdown
Member

Xuanwo commented Oct 21, 2023

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

@xyjixyjixyji
Copy link
Copy Markdown
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
@github-actions github-actions Bot added the releases-note/fix The PR fixes a bug or has a title that begins with "fix" label Oct 21, 2023
@xyjixyjixyji
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Labels

releases-note/fix The PR fixes a bug or has a title that begins with "fix"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants