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

Incorrect behavior about AsyncSeek of TokioCompatFile #80

Closed
silver-ymz opened this issue May 16, 2023 · 6 comments · Fixed by #81
Closed

Incorrect behavior about AsyncSeek of TokioCompatFile #80

silver-ymz opened this issue May 16, 2023 · 6 comments · Fixed by #81
Assignees
Labels
bug Something isn't working

Comments

@silver-ymz
Copy link
Member

silver-ymz commented May 16, 2023

Sometimes AsyncSeek to TokioCompatFile will set the offset to the wrong place. This error occurs in apache/opendal#2263 https://github.com/apache/incubator-opendal/actions/runs/4976970726/jobs/8905529556

I guess the problem is about

fn consume(self: Pin<&mut Self>, amt: usize) {
let this = self.project();
let buffer = this.buffer;
let amt = min(buffer.len(), amt);
buffer.advance(amt);
this.inner.offset += amt as u64;
}

When the offset amount is larger than buffer.len(), it will only offset to the end of buffer.

I haven't found the code which can reproduce stably. I will try to fix it these days.

@silver-ymz silver-ymz added the bug Something isn't working label May 16, 2023
@silver-ymz silver-ymz self-assigned this May 16, 2023
@NobodyXu
Copy link
Member

Note that File::start_seek does not support SeekFrom::End, since I'm not sure how to implement it in a race-free manner

@NobodyXu
Copy link
Member

Also, in

offset can be larger than the self.buffer.len(), this is a UB according to AsyncBufRead interface.

@NobodyXu
Copy link
Member

However, our AsyncBufRead impl is also wrong

fn consume(self: Pin<&mut Self>, amt: usize) {
let this = self.project();
let buffer = this.buffer;
let amt = min(buffer.len(), amt);
buffer.advance(amt);
this.inner.offset += amt as u64;
}

It should panic if amt is more than self.buffer.len()

@NobodyXu
Copy link
Member

What I suggest you to do is to advance the buffer and increase offset manually in

@silver-ymz
Copy link
Member Author

What I suggest you to do is to advance the buffer and increase offset manually in

I think so. I'll complete it tomorrow.

@NobodyXu
Copy link
Member

What I suggest you to do is to advance the buffer and increase offset manually in

I think so. I'll complete it tomorrow.

It will also be great if you can fix ::consume

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

2 participants