felipecrv commented on code in PR #40119:
URL: https://github.com/apache/arrow/pull/40119#discussion_r1494563713
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -698,11 +721,15 @@ class ObjectAppendStream final : public io::OutputStream {
ObjectAppendStream(std::shared_ptr<Blobs::BlockBlobClient> block_blob_client,
const io::IOContext& io_context, const AzureLocation&
location,
const std::shared_ptr<const KeyValueMetadata>& metadata,
- const AzureOptions& options, int64_t size = kNoSize)
+ const AzureOptions& options, const bool truncate,
+ std::function<Status()>
ensure_not_flat_namespace_directory,
Review Comment:
By injecting this `std::function` here you carry the `this` pointer that it
caputers when created — if the filesystem gets destroyed before the
`ObjectAppendStream` we could be referring to a destroyed `AzureFileSystem`
instance.
The cleaner solution is to inject a `shared_ptr<AzureFileSystem>` here. You
can get one by calling `AzureFileSystem->shared_from_this()` (all
`arrow::FileSystem` extends `std::enable_shared_from_this<arrow::FileSystem>`)
and casting it from `shared_ptr<FileSystem>` to `shared_ptr<AzureFileSystem>`.
And in `azurefs.h` you should forward-declare `ObjectAppendStream` and declare
it a friend of `AzureFileSystem` so then it has access to the
`AzureFileSystem::Impl` here.
Having the `ObjectAppendStream` holding a shared_ptr to the filesystem means
the file system will only be destroyed when all the stream instances are also
destroyed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
