You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "dimbtp (via GitHub)" <gi...@apache.org> on 2024/03/18 10:36:34 UTC
[PR] fix: copy/rename return error if source is nonexistent [arrow-rs]
dimbtp opened a new pull request, #5528:
URL: https://github.com/apache/arrow-rs/pull/5528
# Which issue does this PR close?
Closes #5503.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: copy/rename return error if source is nonexistent [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5528:
URL: https://github.com/apache/arrow-rs/pull/5528#discussion_r1529021884
##########
object_store/src/local.rs:
##########
@@ -600,6 +600,14 @@ impl ObjectStore for LocalFileSystem {
async fn copy(&self, from: &Path, to: &Path) -> Result<()> {
let from = self.path_to_filesystem(from)?;
+ if !from.exists() {
+ return Err(Error::NotFound {
Review Comment:
This suffers from a TOCTOU race condition, I think we need to instead detect and handle the case of parent directories already existing when we go to create them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: copy/rename return error if source is nonexistent [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5528:
URL: https://github.com/apache/arrow-rs/pull/5528#discussion_r1532488631
##########
object_store/src/local.rs:
##########
@@ -990,6 +999,7 @@ mod tests {
list_with_delimiter(&integration).await;
rename_and_copy(&integration).await;
copy_if_not_exists(&integration).await;
+ copy_rename_nonexistent_object(&integration).await;
Review Comment:
Yes, I intended to do this as a follow up but would also be happy to review a PR that did this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: copy/rename return error if source is nonexistent [arrow-rs]
Posted by "carols10cents (via GitHub)" <gi...@apache.org>.
carols10cents commented on code in PR #5528:
URL: https://github.com/apache/arrow-rs/pull/5528#discussion_r1532115476
##########
object_store/src/local.rs:
##########
@@ -990,6 +999,7 @@ mod tests {
list_with_delimiter(&integration).await;
rename_and_copy(&integration).await;
copy_if_not_exists(&integration).await;
+ copy_rename_nonexistent_object(&integration).await;
Review Comment:
@tustvold should this test function be called in all the `ObjectStore` implementations' tests now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: copy/rename return error if source is nonexistent [arrow-rs]
Posted by "carols10cents (via GitHub)" <gi...@apache.org>.
carols10cents commented on PR #5528:
URL: https://github.com/apache/arrow-rs/pull/5528#issuecomment-2009610612
Thank you for fixing this @dimbtp !!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: copy/rename return error if source is nonexistent [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #5528:
URL: https://github.com/apache/arrow-rs/pull/5528
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: copy/rename return error if source is nonexistent [arrow-rs]
Posted by "dimbtp (via GitHub)" <gi...@apache.org>.
dimbtp commented on PR #5528:
URL: https://github.com/apache/arrow-rs/pull/5528#issuecomment-2005683089
Thanks for your review.
What do you think about these two options:
- try to read the file directly instead of checking if the file exists
```rust
if let Err(source) = OpenOptions::new().read(true).open(&path) {
// ...
}
```
- put file exist check in `maybe_spawn_blocking`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: copy/rename return error if source is nonexistent [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5528:
URL: https://github.com/apache/arrow-rs/pull/5528#issuecomment-2007910797
So thinking about this a bit more, I think the racyness shouldn't matter, as we'll just loop round and try again. Something like this should work:
```
ErrorKind::NotFound => match from.exists() {
true => create_parent_dirs(&to, source)?,
false => return Err(Error::NotFound { path: from, source }.into()),
},
```
This pattern can then be applied to `rename`, `copy` and `copy_if_not_exists`, with tests of the same
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: copy/rename return error if source is nonexistent [arrow-rs]
Posted by "dimbtp (via GitHub)" <gi...@apache.org>.
dimbtp commented on PR #5528:
URL: https://github.com/apache/arrow-rs/pull/5528#issuecomment-2008663526
It makes sense and thank you for explanation.Ready for another review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org