ARROW-1500: [C++] Do not ignore return value from truncate in MemoryMa… #1116
Conversation
cpp/src/arrow/io/file.cc
Outdated
| std::stringstream ss; | ||
| ss << "Unable to create file, error: " << std::strerror(errno); | ||
| return Status::IOError(ss.str()); | ||
| } |
There was a problem hiding this comment.
Can you use the CheckOpenResult function for this? Adding the errno to the error string generated there would be useful
There was a problem hiding this comment.
Ack. Perhaps I should rename "CheckOpenResult" to something more generic like "CheckFileOpsResult"?
|
The PR title is missing an "A" in |
amirma
changed the title
cpp/src/arrow/io/file.cc
Outdated
| std::stringstream ss; | ||
| ss << "Failed to open local file: " << file_name.string(); | ||
| ss << "Failed to " << opname << " file: " << file_name.string(); | ||
| ss << " ,error: " << std::strerror(errno_actual); |
There was a problem hiding this comment.
Done. Thanks for reviewing!
cpp/src/arrow/io/file.cc
Outdated
| std::stringstream ss; | ||
| ss << "Failed to open local file: " << file_name.string(); | ||
| ss << "Failed to " << opname << " file: " << file_name.string(); | ||
| ss << " ,error: " << std::strerror(errno_actual); |
There was a problem hiding this comment.
Done. Thanks for reviewing!
|
Thanks, the Travis CI tubes are a bit clogged today so I may not be able to merge until later tonight or tomorrow morning |
|
@wesm Bah, I just noticed my patch has a bug; if truncate fails we will leak the file handle. I just resubmitted a fixed version. Thanks. |
amirma
force-pushed
the
arrow-1500
branch
2 times, most recently
from
cf6e78e
to
15c5231
Compare
cpp/src/arrow/io/file.cc
Outdated
| auto truncate_result = CheckFileOpResult(ret, errno_actual, path, "truncate"); | ||
|
|
||
| // First try to close the file before checking the result of | ||
| // truncate, so we don't leak the file handle, if truncate failed | ||
| RETURN_NOT_OK(file->Close()); |
There was a problem hiding this comment.
I think this would be handled by RAII without this https://github.com/apache/arrow/blob/master/cpp/src/arrow/io/file.cc#L473. If there is an esoteric failure to close the file handle then this would bubble up the error, but I'm not sure why that would occur
There was a problem hiding this comment.
Right, I missed that line in the dtor. Then I'll revert to the previous version of the patch.
|
Can you rebase? Not sure why there's a merge conflict now |
|
Rebased. |
|
This is failing with cpplint warnings you can use |
…appedFile::Create Author: Amir Malekpour <a.malekpour@gmail.com>
|
Fixed lint errors. |
…ppedFile::Create
Author: Amir Malekpour a.malekpour@gmail.com