Skip to content

GH-44071: [C++] Leak S3 structures if finalization happens too late - #44090

Merged
pitrou merged 1 commit into
apache:mainfrom
pitrou:gh44071-s3-finalize-too-late
Sep 23, 2024
Merged

GH-44071: [C++] Leak S3 structures if finalization happens too late#44090
pitrou merged 1 commit into
apache:mainfrom
pitrou:gh44071-s3-finalize-too-late

Conversation

@pitrou

@pitroupitrou commented Sep 12, 2024

Copy link
Copy Markdown
Member

Rationale for this change

Leaking S3 structures at shutdown can be better than inducing a segfault because those structures' destructors run too late at process exit.

This seems to avoid the crash when run under uwsgi in #44071

Are these changes tested?

Yes.

Are there any user-facing changes?

Hopefully not.

@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit -g cpp

@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit wheelcp312*

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@pitrou

This comment was marked as outdated.

@github-actions

Copy link
Copy Markdown

Revision: e7d9ea53df7292ec7a0016778d9679537f7ffd7e

Submitted crossbow builds: ursacomputing/crossbow @ actions-de41ec5070

TaskStatus
r-binary-packagesGitHub Actions
test-r-arrow-backwards-compatibilityGitHub Actions
test-r-clang-sanitizerGitHub Actions
test-r-depsource-bundledAzure
test-r-depsource-systemGitHub Actions
test-r-dev-duckdbGitHub Actions
test-r-devdocsGitHub Actions
test-r-extra-packagesGitHub Actions
test-r-gcc-11GitHub Actions
test-r-gcc-12GitHub Actions
test-r-install-localGitHub Actions
test-r-install-local-minsizerelGitHub Actions
test-r-linux-as-cranGitHub Actions
test-r-linux-rchkGitHub Actions
test-r-linux-valgrindGitHub Actions
test-r-macos-as-cranGitHub Actions
test-r-minimal-buildAzure
test-r-offline-maximalGitHub Actions
test-r-offline-minimalAzure
test-r-rhub-debian-gcc-devel-lto-latestAzure
test-r-rhub-debian-gcc-release-custom-ccacheAzure
test-r-rhub-ubuntu-release-latestAzure
test-r-rocker-r-ver-latestAzure
test-r-rstudio-r-base-4.1-opensuse155Azure
test-r-rstudio-r-base-4.2-focalAzure
test-r-ubuntu-22.04GitHub Actions
test-r-versionsGitHub Actions
test-ubuntu-r-sanitizerGitHub Actions

@pitrou

pitrou commented Sep 12, 2024

Copy link
Copy Markdown
MemberAuthor

Need #44093 before we can properly test S3 on Windows wheel builds

Edit: it's now merged

@pitrou
pitrouforce-pushed the gh44071-s3-finalize-too-late branch from e7d9ea5 to 052e196CompareSeptember 13, 2024 14:14
@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit wheelcp312*

@github-actions

This comment was marked as outdated.

@pitrou

pitrou commented Sep 13, 2024

Copy link
Copy Markdown
MemberAuthor

I notice that S3 is similarly untested on macOS Python builds: #44111

Edit: fixed now

@pitrou
pitrouforce-pushed the gh44071-s3-finalize-too-late branch from 052e196 to 236c551CompareSeptember 16, 2024 12:07
@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit wheel-macos*

@github-actions

This comment was marked as outdated.

@pitrou

Copy link
Copy Markdown
MemberAuthor

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

@apacheapache deleted a comment from github-actionsBotSep 16, 2024
@github-actions

This comment was marked as outdated.

@pitrou

pitrou commented Sep 16, 2024

Copy link
Copy Markdown
MemberAuthor

This is very hackish but solves the issue of using PyArrow under uwsgi. @felipecrv What is your opinion?

Edit: it did but it doesn't. I don't even understand what has changed since a couple hours ago... Now fixed hopefully.

@pitrou
pitrouforce-pushed the gh44071-s3-finalize-too-late branch from 236c551 to 380c531CompareSeptember 16, 2024 16:38
@pitrou

Copy link
Copy Markdown
MemberAuthor

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

@github-actions

This comment was marked as outdated.

@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit wheelmacos wheellinux

@github-actions

This comment was marked as outdated.

@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit -g python

@github-actions

This comment was marked as outdated.

@pitrou
pitrouforce-pushed the gh44071-s3-finalize-too-late branch from 5589451 to c078ac5CompareSeptember 16, 2024 17:18
@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit wheelmacoscp312* wheellinuxcp312* wheelmacoscp313* wheellinuxcp313*

@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit -g wheel

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@pitrou
pitrouforce-pushed the gh44071-s3-finalize-too-late branch from d997350 to 8946c1aCompareSeptember 17, 2024 08:26
@pitrou

Copy link
Copy Markdown
MemberAuthor

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

@github-actions

This comment was marked as outdated.

@pitrou
pitrou marked this pull request as ready for review September 17, 2024 11:40
@pitroupitrou changed the title EXPERIMENT: GH-44071: [C++] Leak S3 structures if finalization happens too lateGH-44071: [C++] Leak S3 structures if finalization happens too lateSep 17, 2024
@pitrou

Copy link
Copy Markdown
MemberAuthor

The C/GLib failure looks unrelated:
https://github.com/apache/arrow/actions/runs/10899317850/job/30247191830?pr=44090#step:9:270

../../c_glib/arrow-flight-glib/client.cpp:278: Warning: ArrowFlight: invalid "closure" annotation: only valid on callback parameters
../../c_glib/arrow-flight-glib/server.cpp:465: Warning: ArrowFlight: invalid "closure" annotation: only valid on callback parameters
<unknown>:: Fatal: ArrowFlight: warnings configured as fatal
<unknown>:: Fatal: ArrowFlight: warnings configured as fatal

What do you think @kou ?

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #44071has been automatically assigned in GitHub to PR creator.

@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit -g cpp

@pitrou

Copy link
Copy Markdown
MemberAuthor

@jorisvandenbossche Can you take a look at the Python test?

@github-actions

Copy link
Copy Markdown

Revision: 8946c1a9be668a1c53df9e61c75119553c009be7

Submitted crossbow builds: ursacomputing/crossbow @ actions-60301353eb

TaskStatus
example-cpp-minimal-build-staticGitHub Actions
example-cpp-minimal-build-static-system-dependencyGitHub Actions
example-cpp-tutorialGitHub Actions
test-alpine-linux-cppGitHub Actions
test-build-cpp-fuzzGitHub Actions
test-conda-cppGitHub Actions
test-conda-cpp-valgrindGitHub Actions
test-cuda-cpp-ubuntu-20.04-cuda-11.2.2GitHub Actions
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1GitHub Actions
test-debian-12-cpp-amd64GitHub Actions
test-debian-12-cpp-i386GitHub Actions
test-fedora-39-cppGitHub Actions
test-ubuntu-20.04-cppGitHub Actions
test-ubuntu-20.04-cpp-bundledGitHub Actions
test-ubuntu-20.04-cpp-minimal-with-formatsGitHub Actions
test-ubuntu-20.04-cpp-thread-sanitizerGitHub Actions
test-ubuntu-22.04-cppGitHub Actions
test-ubuntu-22.04-cpp-20GitHub Actions
test-ubuntu-22.04-cpp-emscriptenGitHub Actions
test-ubuntu-22.04-cpp-no-threadingGitHub Actions
test-ubuntu-24.04-cppGitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundledGitHub Actions
test-ubuntu-24.04-cpp-gcc-14GitHub Actions

@kou

kou commented Sep 18, 2024

Copy link
Copy Markdown
Member

The C/GLib failure looks unrelated: https://github.com/apache/arrow/actions/runs/10899317850/job/30247191830?pr=44090#step:9:270

../../c_glib/arrow-flight-glib/client.cpp:278: Warning: ArrowFlight: invalid "closure" annotation: only valid on callback parameters
../../c_glib/arrow-flight-glib/server.cpp:465: Warning: ArrowFlight: invalid "closure" annotation: only valid on callback parameters
<unknown>:: Fatal: ArrowFlight: warnings configured as fatal
<unknown>:: Fatal: ArrowFlight: warnings configured as fatal

What do you think @kou ?

It's unrelated. I'll fix it by #44153.

@kou

kou commented Sep 18, 2024

Copy link
Copy Markdown
Member

Fixed.

@pitrou
pitrouforce-pushed the gh44071-s3-finalize-too-late branch from 8946c1a to 07cc766CompareSeptember 19, 2024 12:42
@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit wheelmacos wheellinux

@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit -g python

@github-actions

Copy link
Copy Markdown

Revision: 07cc766

Submitted crossbow builds: ursacomputing/crossbow @ actions-4f61f433c5

TaskStatus
wheel-macos-monterey-cp310-cp310-amd64GitHub Actions
wheel-macos-monterey-cp310-cp310-arm64GitHub Actions
wheel-macos-monterey-cp311-cp311-amd64GitHub Actions
wheel-macos-monterey-cp311-cp311-arm64GitHub Actions
wheel-macos-monterey-cp312-cp312-amd64GitHub Actions
wheel-macos-monterey-cp312-cp312-arm64GitHub Actions
wheel-macos-monterey-cp313-cp313-amd64GitHub Actions
wheel-macos-monterey-cp313-cp313-arm64GitHub Actions
wheel-macos-monterey-cp313-cp313t-amd64GitHub Actions
wheel-macos-monterey-cp313-cp313t-arm64GitHub Actions
wheel-macos-monterey-cp39-cp39-amd64GitHub Actions
wheel-macos-monterey-cp39-cp39-arm64GitHub Actions
wheel-manylinux-2-28-cp310-cp310-amd64GitHub Actions
wheel-manylinux-2-28-cp310-cp310-arm64GitHub Actions
wheel-manylinux-2-28-cp311-cp311-amd64GitHub Actions
wheel-manylinux-2-28-cp311-cp311-arm64GitHub Actions
wheel-manylinux-2-28-cp312-cp312-amd64GitHub Actions
wheel-manylinux-2-28-cp312-cp312-arm64GitHub Actions
wheel-manylinux-2-28-cp313-cp313-amd64GitHub Actions
wheel-manylinux-2-28-cp313-cp313-arm64GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-amd64GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-arm64GitHub Actions
wheel-manylinux-2-28-cp39-cp39-amd64GitHub Actions
wheel-manylinux-2-28-cp39-cp39-arm64GitHub Actions
wheel-manylinux-2014-cp310-cp310-amd64GitHub Actions
wheel-manylinux-2014-cp310-cp310-arm64GitHub Actions
wheel-manylinux-2014-cp311-cp311-amd64GitHub Actions
wheel-manylinux-2014-cp311-cp311-arm64GitHub Actions
wheel-manylinux-2014-cp312-cp312-amd64GitHub Actions
wheel-manylinux-2014-cp312-cp312-arm64GitHub Actions
wheel-manylinux-2014-cp313-cp313-amd64GitHub Actions
wheel-manylinux-2014-cp313-cp313-arm64GitHub Actions
wheel-manylinux-2014-cp313-cp313t-amd64GitHub Actions
wheel-manylinux-2014-cp313-cp313t-arm64GitHub Actions
wheel-manylinux-2014-cp39-cp39-amd64GitHub Actions
wheel-manylinux-2014-cp39-cp39-arm64GitHub Actions

@github-actions

Copy link
Copy Markdown

Revision: 07cc766

Submitted crossbow builds: ursacomputing/crossbow @ actions-7df153a302

TaskStatus
example-python-minimal-build-fedora-condaGitHub Actions
example-python-minimal-build-ubuntu-venvGitHub Actions
test-conda-python-3.10GitHub Actions
test-conda-python-3.10-cython2GitHub Actions
test-conda-python-3.10-hdfs-2.9.2GitHub Actions
test-conda-python-3.10-hdfs-3.2.1GitHub Actions
test-conda-python-3.10-pandas-latest-numpy-latestGitHub Actions
test-conda-python-3.10-substraitGitHub Actions
test-conda-python-3.11GitHub Actions
test-conda-python-3.11-dask-latestGitHub Actions
test-conda-python-3.11-dask-upstream_develGitHub Actions
test-conda-python-3.11-hypothesisGitHub Actions
test-conda-python-3.11-pandas-latest-numpy-1.26GitHub Actions
test-conda-python-3.11-pandas-latest-numpy-latestGitHub Actions
test-conda-python-3.11-pandas-nightly-numpy-nightlyGitHub Actions
test-conda-python-3.11-pandas-upstream_devel-numpy-nightlyGitHub Actions
test-conda-python-3.11-spark-masterGitHub Actions
test-conda-python-3.12GitHub Actions
test-conda-python-3.12-cpython-debugGitHub Actions
test-conda-python-3.9GitHub Actions
test-conda-python-3.9-pandas-1.1.3-numpy-1.19.5GitHub Actions
test-conda-python-emscriptenGitHub Actions
test-cuda-python-ubuntu-22.04-cuda-11.7.1GitHub Actions
test-debian-12-python-3-amd64GitHub Actions
test-debian-12-python-3-i386GitHub Actions
test-fedora-39-python-3GitHub Actions
test-ubuntu-22.04-python-3GitHub Actions
test-ubuntu-22.04-python-313-freethreadingGitHub Actions
test-ubuntu-24.04-python-3GitHub Actions

@pitrou

Copy link
Copy Markdown
MemberAuthor

CI is green. @jorisvandenbossche Are you available to take a quick look at the Python test?

@pitrou
pitrou merged commit 0f7b5e5 into apache:mainSep 23, 2024
@pitroupitrou removed the awaiting merge Awaiting merge label Sep 23, 2024
@pitrou
pitrou deleted the gh44071-s3-finalize-too-late branch September 23, 2024 08:30
@jorisvandenbossche

Copy link
Copy Markdown
Member

Are you available to take a quick look at the Python test?

Looks good!

@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 0f7b5e5.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@pitrou@kou@jorisvandenbossche@felipecrv