GH-36346: [C++] Safe S3 finalization by pitrou · Pull Request #36442 · apache/arrow

pitrou · 2023-07-03T17:12:54Z

Rationale for this change

It is required to call FinalizeS3 at process end to cleanly shut down resources used by the AWS SDK before it's too late.
However, once FinalizeS3 has been called, another problem appears: no AWS SDK API can be called safely anymore. Even object destructors can be dangerous.
We therefore need a way to prevent unsafe use of the AWS SDK APIs, regardless of how the application issues filesystems calls.

What changes are included in this PR?

Shield all uses of the internal S3Client class behind a safe RAII facility S3ClientLock. Obtaining a S3ClientLock ensures that S3 finalization has not been called yet and that it will not be called until the S3ClientLock goes out of scope.

Are these changes tested?

Yes, some Python tests exercise calling S3 APIs after explicit finalization.

Are there any user-facing changes?

Not for correctly written application code, ideally.

Closes: [C++][S3] Crash on exit #36346

pitrou · 2023-07-03T17:13:21Z

This is an alternative approach to #36437. @westonpace @kou

pitrou · 2023-07-03T17:17:35Z

Note that I don't think this should be a problem in practice, as calls to EnsureInitialized and Finalize should be serialized at the application level.

I agree with your opinion.
How about adding this note to this comment or documents for InitializeS3()/FinalizeS3()?
(I think that we can remove the "XXX" mark here.)

pitrou · 2023-07-03T17:39:15Z

@github-actions crossbow submit -g python -g wheel

kou · 2023-07-04T03:05:51Z

I agree with your opinion.
How about adding this note to this comment or documents for InitializeS3()/FinalizeS3()?
(I think that we can remove the "XXX" mark here.)

pitrou · 2023-07-04T09:16:38Z

@github-actions crossbow submit -g python -g wheel

pitrou · 2023-07-04T09:18:42Z

Thanks a lot for the review @kou . I agree with all your comments and made the necessary changes.

github-actions · 2023-07-04T09:21:05Z

Revision: c171b31

Submitted crossbow builds: ursacomputing/crossbow @ actions-1c2290f238

Task	Status
test-conda-python-3.10
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-master
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.1.2
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-conda-python-3.9-spark-v3.2.0
test-cuda-python
test-debian-11-python-3
test-fedora-35-python-3
test-ubuntu-20.04-python-3
wheel-clean
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-mojave-cp310-amd64
wheel-macos-mojave-cp311-amd64
wheel-macos-mojave-cp38-amd64
wheel-macos-mojave-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

pitrou · 2023-07-04T09:42:18Z

@github-actions crossbow submit -g cpp

github-actions · 2023-07-04T09:45:07Z

Revision: c171b31

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

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-20
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

pitrou · 2023-07-04T11:42:21Z

The remaining CI failures seem unrelated. I think I'm going to merge so as to improve CI, as we're nearing the release.

conbench-apache-arrow · 2023-07-08T08:17:01Z

Conbench analyzed the 6 benchmark runs on commit e7d5028d.

There were 7 benchmark results indicating a performance regression:

Commit Run on arm64-m6g-linux-compute at 2023-07-04 14:27:07Z
- params=<STATIC_VECTOR(int)>, source=cpp-micro, suite=arrow-small-vector-benchmark
- params=<SMALL_VECTOR(std::string)>, source=cpp-micro, suite=arrow-small-vector-benchmark
and 5 more (see the report linked below)

The full Conbench report has more details.

### Rationale for this change In #12227 we decided to use a bundled version of the AWS SDK when compiling Python wheels, in order to downgrade the AWS SDK version. Now that we have fixed S3 finalization issues (#36442), it should be ok to rely on the vcpkg-installed version of the AWS SDK again. ### What changes are included in this PR? Remove use of bundled AWS SDK and use S3 vcpkg feature for requirements. ### Are these changes tested? On CI and via crossbow ### Are there any user-facing changes? No * Closes: #36752 Authored-by: Raúl Cumplido <raulcumplido@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

hellkite500 · 2023-08-23T17:14:29Z

Did this make it into v13.0.0? If so, I'm still seeing issues

/Users/voltrondata/github-actions-runner/_work/crossbow/crossbow/arrow/cpp/src/arrow/filesystem/s3fs.cc:2829: arrow::fs::FinalizeS3 was not called even though S3 was initialized. This could lead to a segmentation fault at exit
zsh: segmentation fault ./test_routing_pybind
(venv) test % pip list | grep pyarrow
pyarrow 13.0.0

pitrou · 2023-08-23T17:15:37Z

Yes, it should be in 13.0.0. Can you please open an issue with a reproducer script?

…apache#36925) ### Rationale for this change In apache#12227 we decided to use a bundled version of the AWS SDK when compiling Python wheels, in order to downgrade the AWS SDK version. Now that we have fixed S3 finalization issues (apache#36442), it should be ok to rely on the vcpkg-installed version of the AWS SDK again. ### What changes are included in this PR? Remove use of bundled AWS SDK and use S3 vcpkg feature for requirements. ### Are these changes tested? On CI and via crossbow ### Are there any user-facing changes? No * Closes: apache#36752 Authored-by: Raúl Cumplido <raulcumplido@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

github-actionsBot added Component: C++ Component: Python labels Jul 3, 2023

pitrou mentioned this pull request Jul 3, 2023
GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize #36437
Closed

github-actionsBot added the awaiting review Awaiting review label Jul 3, 2023

pitrou commented Jul 3, 2023
View reviewed changes

github-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 3, 2023

This comment was marked as outdated.
Sign in to view

kou reviewed Jul 4, 2023
View reviewed changes

github-actionsBot added awaiting review Awaiting review awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 4, 2023

apacheGH-36346: [C++] Safe S3 finalization
c171b31

pitrou force-pushed the s3-finalize-safe branch from 051209d to c171b31Compare July 4, 2023 09:16

pitrou marked this pull request as ready for review July 4, 2023 09:16

pitrou requested a review from AlenkaF as a code owner July 4, 2023 09:16

github-actionsBot added awaiting review Awaiting review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Jul 4, 2023

pitrou mentioned this pull request Jul 4, 2023
[C++][CI] arrow-s3fs-test fails on some nightly jobs #36447
Closed

This was referenced Jul 4, 2023
Crash on exist with pyarrow >= 12.0.0 NOAA-OWP/t-route#614
Closed
[Bug]: Loading from S3 bucket got segment fault error apache/incubator-graphar#187
Closed

Mention GH issues in comment
2f902ee

pitrou merged commit e7d5028 into apache:mainJul 4, 2023

pitrou removed the awaiting review Awaiting review label Jul 4, 2023

pitrou deleted the s3-finalize-safe branch July 4, 2023 12:47

pitrou mentioned this pull request Jul 18, 2023
[Python] Remove AWS SDK bundling when building wheels #36752
Closed

mapleFU mentioned this pull request Jul 23, 2023
[Java] while using s3 FileSystemDatasetFactory getting this exception #36069
Closed

raulcd mentioned this pull request Jul 28, 2023
GH-36752: [Python] Remove AWS SDK bundling when building wheels #36925
Merged

Uh oh!

Conversation

pitrou commented Jul 3, 2023• edited by github-actions BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

pitrou commented Jul 3, 2023

Uh oh!

pitrouJul 3, 2023

Choose a reason for hiding this comment

Uh oh!

kouJul 4, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jul 3, 2023

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kouJul 4, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jul 4, 2023

Uh oh!

pitrou commented Jul 4, 2023

Uh oh!

github-actionsBot commented Jul 4, 2023

Uh oh!

pitrou commented Jul 4, 2023

Uh oh!

github-actionsBot commented Jul 4, 2023

Uh oh!

pitrou commented Jul 4, 2023

Uh oh!

conbench-apache-arrowBot commented Jul 8, 2023

Uh oh!

hellkite500 commented Aug 23, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Aug 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pitrou commented Jul 3, 2023•
edited by github-actions Bot
Loading

hellkite500 commented Aug 23, 2023•
edited
Loading