arrow GH-40036: [C++] Azure file system write buffering & async writes

Rationale for this change

See #40036.

What changes are included in this PR?

Write buffering and async writes (similar to what the S3 file system does) in the ObjectAppendStream for the Azure file system.

With write buffering and async writes, the input scenario creation runtime in the tests (which uses the ObjectAppendStream against Azurite) decreased from ~25s (see here) to ~800ms:

[ RUN      ] TestAzuriteFileSystem.OpenInputFileMixedReadVsReadAt
[       OK ] TestAzuriteFileSystem.OpenInputFileMixedReadVsReadAt (787 ms)

Are these changes tested?

Added some tests with background writes enabled and disabled (some were taken from the S3 tests). Everything changed should be covered.

Are there any user-facing changes?

AzureOptions now allows for background_writes to be set (default: true). No breaking changes.

Notes

The code in DoWrite is very similar to the code in the S3 FS. Maybe this could be unified? I didn't see this in the scope of the PR though.

GitHub Issue: #40036

Jul 01 '24 14:07 OliLay

:warning: GitHub issue #40036 has been automatically assigned in GitHub to PR creator.

Jul 01 '24 14:07 github-actions[bot]

Hi @kou, sorry for directly pinging you again, but do you maybe know who would be an appropriate person to look at this PR? Same for #43098

Jul 18 '24 14:07 OliLay

No problem. Sorry for not reviewing this and https://github.com/apache/arrow/pull/43098. @Tom-Newton @felipecrv and I implemented Azure filesystem. So one of us are appropriate person to ping.

Jul 19 '24 01:07 kou

The code in DoWrite is very similar to the code in the S3 FS. Maybe this could be unified? I didn't see this in the scope of the PR though.

It's better if it improve maintainability. We can work on it as a follow-up task.

Yeah, I just have to admit that I haven't come up with a good idea to factor this out and provide a good abstraction on top. Because this functionality may be needed by any file system that implements write buffering.

Jul 19 '24 07:07 OliLay

@Tom-Newton can you take a look?

Aug 01 '24 15:08 felipecrv

@Tom-Newton can you take a look?

I can try. Maybe I can look at it tomorrow evening

Aug 01 '24 16:08 Tom-Newton

Thanks for working on this @OliLay. Hopefully my review is useful - I am a C++ noob 😅

Thanks for your review, it is much appreciated :+1:

Aug 06 '24 13:08 OliLay

@pitrou Could you review this?

Aug 15 '24 00:08 kou

A small (probably innocuous) leak in reliably reported by the ASAN job. I am narrowing it down and trying to come up with a workaround:


==13823==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1936 byte(s) in 2 object(s) allocated from:
    #0 0x565171d11708 in calloc (/build/cpp/debug/arrow-azurefs-test+0x4c8708) (BuildId: 308c17e28257859d0082a84d74ef009b3a4122a8)
    #0 0x7f72301e95a4 in

Aug 21 '24 09:08 pitrou

A more detailed leak trace (obtained using LSAN_OPTIONS=fast_unwind_on_malloc=0:malloc_context_size=100) is the following:

Direct leak of 968 byte(s) in 1 object(s) allocated from:
    #0 0x5600b66641e8 in calloc (/build/cpp/debug/arrow-azurefs-test+0x4c31e8) (BuildId: 5f129cb231bb2651f5fb432f4c8d3c2f40506da3)
    #1 0x7f85322b25a4  (/lib/x86_64-linux-gnu/libxml2.so.2+0xe25a4) (BuildId: aebf8e42966c3ce475ff9d9d51a762831adcbb61)
    #2 0x7f85322a5fb4 in __xmlDefaultBufferSize (/lib/x86_64-linux-gnu/libxml2.so.2+0xd5fb4) (BuildId: aebf8e42966c3ce475ff9d9d51a762831adcbb61)
    #3 0x7f8532240d37 in xmlBufferCreate (/lib/x86_64-linux-gnu/libxml2.so.2+0x70d37) (BuildId: aebf8e42966c3ce475ff9d9d51a762831adcbb61)
    #4 0x7f8547ec6f74 in Azure::Storage::_internal::XmlWriter::XmlWriter() /build/cpp/_deps/azure_sdk-src/sdk/storage/azure-storage-common/src/xml_wrapper.cpp:532:19
    #5 0x7f8547e87621 in Azure::Storage::Blobs::_detail::BlockBlobClient::CommitBlockList(Azure::Core::Http::_internal::HttpPipeline&, Azure::Core::Url const&, Azure::Storage::Blobs::_detail::BlockBlobClient::CommitBlockBlobBlockListOptions const&, Azure::Core::Context const&) /build/cpp/_deps/azure_sdk-src/sdk/storage/azure-storage-blobs/src/rest_client.cpp:7944:30
    #6 0x7f8547de3149 in Azure::Storage::Blobs::BlockBlobClient::CommitBlockList(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, Azure::Storage::Blobs::CommitBlockListOptions const&, Azure::Core::Context const&) const /build/cpp/_deps/azure_sdk-src/sdk/storage/azure-storage-blobs/src/block_blob_client.cpp:512:12
    #7 0x7f854618c14c in arrow::fs::(anonymous namespace)::CommitBlockList(std::shared_ptr<Azure::Storage::Blobs::BlockBlobClient>, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, Azure::Storage::Blobs::CommitBlockListOptions const&) /arrow/cpp/src/arrow/filesystem/azurefs.cc:948:24
    #8 0x7f8546193aab in arrow::fs::(anonymous namespace)::ObjectAppendStream::FlushAsync()::'lambda'()::operator()() const /arrow/cpp/src/arrow/filesystem/azurefs.cc:1158:14
    #9 0x7f85461933eb in std::enable_if<((!(std::is_void<arrow::Status>::value)) && (!(is_future<arrow::Status>::value))) && ((!(arrow::Future<arrow::internal::Empty>::is_empty)) || (std::is_same<arrow::Status, arrow::Status>::value)), void>::type arrow::detail::ContinueFuture::operator()<arrow::fs::(anonymous namespace)::ObjectAppendStream::FlushAsync()::'lambda'(), arrow::Status, arrow::Future<arrow::internal::Empty> >(arrow::Future<arrow::internal::Empty>, arrow::fs::(anonymous namespace)::ObjectAppendStream::FlushAsync()::'lambda'()&&) const /arrow/cpp/src/arrow/util/future.h:150:23

Aug 21 '24 09:08 pitrou

The leak manifests in TestAzuriteFileSystem.OpenOutputStreamCloseAsync and TestAzuriteFileSystem.OpenOutputStreamAsyncDestructor. However, TestAzuriteFileSystem.OpenOutputStreamAsyncDestructorNoBackgroundWrites is fine.

Aug 21 '24 09:08 pitrou

Interesting, I don't really see where this could come from actually. From the tests that are affected, for me it looks like that only the path when we go through CloseAsync()/FlushAsync() is the issue. The sync and async implementations behave very similar so I don't really see where this could come from, especially since we don't really control the Azure allocations in any way (seeing that the XmlBuffer is apparently the problematic allocation?) In fact, everything Azure related should be deallocated at latest in CloseAsync() when the BlockBlobClient should get destructed because we remove the shared ptr ref.

Aug 21 '24 10:08 OliLay

@OliLay There's already a skip related to a libxml2 leak with threaded operation, so I added another one.

This might actually be fixed by https://github.com/Azure/azure-sdk-for-cpp/pull/5767 which was recently merged in Azure C++.

Also cc @felipecrv

Aug 21 '24 10:08 pitrou

Actually, no, we may need something like this hack from ClickHouse: https://github.com/ClickHouse/ClickHouse/blob/054b38d4ebeabc33010fb532766f937af2d44c03/programs/server/Server.cpp#L987-L998

It was added in this PR: https://github.com/ClickHouse/ClickHouse/pull/45796

Aug 21 '24 10:08 pitrou

@github-actions crossbow submit -g cpp

Aug 21 '24 10:08 pitrou

Revision: a56a34b90bcb6c2911df0d7b61528c6cb148ab4b

Submitted crossbow builds: ursacomputing/crossbow @ actions-d38d42cc6f

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14

Aug 21 '24 10:08 github-actions[bot]

Hmm, I rebased for CI.

Aug 21 '24 10:08 pitrou

@github-actions crossbow submit -g cpp

Aug 21 '24 10:08 pitrou

Revision: d337e09ba00db66fc6ab116483b8b714a14ecf37

Submitted crossbow builds: ursacomputing/crossbow @ actions-0a8f77add1

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14