Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Sep 12, 2024

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
Member Author

pitrou commented Sep 12, 2024

@github-actions crossbow submit -g cpp

@pitrou
Copy link
Member Author

pitrou commented Sep 12, 2024

@github-actions crossbow submit wheelcp312*

This comment was marked as outdated.

This comment was marked as outdated.

@pitrou

This comment was marked as outdated.

Copy link

Revision: e7d9ea5

Submitted crossbow builds: ursacomputing/crossbow @ actions-de41ec5070

Task Status
r-binary-packages GitHub Actions
test-r-arrow-backwards-compatibility GitHub Actions
test-r-clang-sanitizer GitHub Actions
test-r-depsource-bundled Azure
test-r-depsource-system GitHub Actions
test-r-dev-duckdb GitHub Actions
test-r-devdocs GitHub Actions
test-r-extra-packages GitHub Actions
test-r-gcc-11 GitHub Actions
test-r-gcc-12 GitHub Actions
test-r-install-local GitHub Actions
test-r-install-local-minsizerel GitHub Actions
test-r-linux-as-cran GitHub Actions
test-r-linux-rchk GitHub Actions
test-r-linux-valgrind GitHub Actions
test-r-macos-as-cran GitHub Actions
test-r-minimal-build Azure
test-r-offline-maximal GitHub Actions
test-r-offline-minimal Azure
test-r-rhub-debian-gcc-devel-lto-latest Azure
test-r-rhub-debian-gcc-release-custom-ccache Azure
test-r-rhub-ubuntu-release-latest Azure
test-r-rocker-r-ver-latest Azure
test-r-rstudio-r-base-4.1-opensuse155 Azure
test-r-rstudio-r-base-4.2-focal Azure
test-r-ubuntu-22.04 GitHub Actions
test-r-versions GitHub Actions
test-ubuntu-r-sanitizer GitHub Actions

@pitrou
Copy link
Member Author

pitrou commented Sep 12, 2024

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

Edit: it's now merged

@pitrou pitrou force-pushed the gh44071-s3-finalize-too-late branch from e7d9ea5 to 052e196 Compare September 13, 2024 14:14
@pitrou
Copy link
Member Author

pitrou commented Sep 13, 2024

@github-actions crossbow submit wheelcp312*

This comment was marked as outdated.

@pitrou
Copy link
Member Author

pitrou commented Sep 13, 2024

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

Edit: fixed now

@pitrou pitrou force-pushed the gh44071-s3-finalize-too-late branch from 052e196 to 236c551 Compare September 16, 2024 12:07
@pitrou
Copy link
Member Author

pitrou commented Sep 16, 2024

@github-actions crossbow submit wheel-macos*

This comment was marked as outdated.

@pitrou
Copy link
Member Author

pitrou commented Sep 16, 2024

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

@apache apache deleted a comment from github-actions bot Sep 16, 2024

This comment was marked as outdated.

@pitrou
Copy link
Member Author

pitrou commented Sep 16, 2024

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 pitrou force-pushed the gh44071-s3-finalize-too-late branch from 236c551 to 380c531 Compare September 16, 2024 16:38
@pitrou
Copy link
Member Author

pitrou commented Sep 16, 2024

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

This comment was marked as outdated.

@pitrou
Copy link
Member Author

pitrou commented Sep 16, 2024

@github-actions crossbow submit wheelmacos wheellinux

This comment was marked as outdated.

@pitrou
Copy link
Member Author

pitrou commented Sep 16, 2024

@github-actions crossbow submit -g python

This comment was marked as outdated.

@pitrou pitrou force-pushed the gh44071-s3-finalize-too-late branch from 5589451 to c078ac5 Compare September 16, 2024 17:18
@pitrou
Copy link
Member Author

pitrou commented Sep 16, 2024

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

@pitrou
Copy link
Member Author

pitrou commented Sep 17, 2024

@github-actions crossbow submit -g python

@pitrou
Copy link
Member Author

pitrou commented Sep 17, 2024

@github-actions crossbow submit -g wheel

This comment was marked as outdated.

This comment was marked as outdated.

@pitrou pitrou force-pushed the gh44071-s3-finalize-too-late branch from d997350 to 8946c1a Compare September 17, 2024 08:26
@pitrou
Copy link
Member Author

pitrou commented Sep 17, 2024

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

This comment was marked as outdated.

@pitrou pitrou marked this pull request as ready for review September 17, 2024 11:40
@pitrou pitrou changed the title EXPERIMENT: GH-44071: [C++] Leak S3 structures if finalization happens too late GH-44071: [C++] Leak S3 structures if finalization happens too late Sep 17, 2024
@pitrou
Copy link
Member Author

pitrou commented Sep 17, 2024

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 ?

Copy link

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

@pitrou
Copy link
Member Author

pitrou commented Sep 17, 2024

@github-actions crossbow submit -g cpp

@pitrou
Copy link
Member Author

pitrou commented Sep 17, 2024

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

Copy link

Revision: 8946c1a

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

Task Status
example-cpp-minimal-build-static GitHub Actions
example-cpp-minimal-build-static-system-dependency GitHub Actions
example-cpp-tutorial GitHub Actions
test-alpine-linux-cpp GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp-ubuntu-20.04-cuda-11.2.2 GitHub Actions
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1 GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-39-cpp GitHub Actions
test-ubuntu-20.04-cpp GitHub Actions
test-ubuntu-20.04-cpp-bundled GitHub Actions
test-ubuntu-20.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-20.04-cpp-thread-sanitizer GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions

@kou
Copy link
Member

kou commented Sep 18, 2024

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
Copy link
Member

kou commented Sep 18, 2024

Fixed.

@pitrou pitrou force-pushed the gh44071-s3-finalize-too-late branch from 8946c1a to 07cc766 Compare September 19, 2024 12:42
@pitrou
Copy link
Member Author

pitrou commented Sep 19, 2024

@github-actions crossbow submit wheelmacos wheellinux

@pitrou
Copy link
Member Author

pitrou commented Sep 19, 2024

@github-actions crossbow submit -g python

Copy link

Revision: 07cc766

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

Task Status
wheel-macos-monterey-cp310-cp310-amd64 GitHub Actions
wheel-macos-monterey-cp310-cp310-arm64 GitHub Actions
wheel-macos-monterey-cp311-cp311-amd64 GitHub Actions
wheel-macos-monterey-cp311-cp311-arm64 GitHub Actions
wheel-macos-monterey-cp312-cp312-amd64 GitHub Actions
wheel-macos-monterey-cp312-cp312-arm64 GitHub Actions
wheel-macos-monterey-cp313-cp313-amd64 GitHub Actions
wheel-macos-monterey-cp313-cp313-arm64 GitHub Actions
wheel-macos-monterey-cp313-cp313t-amd64 GitHub Actions
wheel-macos-monterey-cp313-cp313t-arm64 GitHub Actions
wheel-macos-monterey-cp39-cp39-amd64 GitHub Actions
wheel-macos-monterey-cp39-cp39-arm64 GitHub Actions
wheel-manylinux-2-28-cp310-cp310-amd64 GitHub Actions
wheel-manylinux-2-28-cp310-cp310-arm64 GitHub Actions
wheel-manylinux-2-28-cp311-cp311-amd64 GitHub Actions
wheel-manylinux-2-28-cp311-cp311-arm64 GitHub Actions
wheel-manylinux-2-28-cp312-cp312-amd64 GitHub Actions
wheel-manylinux-2-28-cp312-cp312-arm64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313-amd64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313-arm64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-amd64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-arm64 GitHub Actions
wheel-manylinux-2-28-cp39-cp39-amd64 GitHub Actions
wheel-manylinux-2-28-cp39-cp39-arm64 GitHub Actions
wheel-manylinux-2014-cp310-cp310-amd64 GitHub Actions
wheel-manylinux-2014-cp310-cp310-arm64 GitHub Actions
wheel-manylinux-2014-cp311-cp311-amd64 GitHub Actions
wheel-manylinux-2014-cp311-cp311-arm64 GitHub Actions
wheel-manylinux-2014-cp312-cp312-amd64 GitHub Actions
wheel-manylinux-2014-cp312-cp312-arm64 GitHub Actions
wheel-manylinux-2014-cp313-cp313-amd64 GitHub Actions
wheel-manylinux-2014-cp313-cp313-arm64 GitHub Actions
wheel-manylinux-2014-cp313-cp313t-amd64 GitHub Actions
wheel-manylinux-2014-cp313-cp313t-arm64 GitHub Actions
wheel-manylinux-2014-cp39-cp39-amd64 GitHub Actions
wheel-manylinux-2014-cp39-cp39-arm64 GitHub Actions

Copy link

Revision: 07cc766

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

Task Status
example-python-minimal-build-fedora-conda GitHub Actions
example-python-minimal-build-ubuntu-venv GitHub Actions
test-conda-python-3.10 GitHub Actions
test-conda-python-3.10-cython2 GitHub Actions
test-conda-python-3.10-hdfs-2.9.2 GitHub Actions
test-conda-python-3.10-hdfs-3.2.1 GitHub Actions
test-conda-python-3.10-pandas-latest-numpy-latest GitHub Actions
test-conda-python-3.10-substrait GitHub Actions
test-conda-python-3.11 GitHub Actions
test-conda-python-3.11-dask-latest GitHub Actions
test-conda-python-3.11-dask-upstream_devel GitHub Actions
test-conda-python-3.11-hypothesis GitHub Actions
test-conda-python-3.11-pandas-latest-numpy-1.26 GitHub Actions
test-conda-python-3.11-pandas-latest-numpy-latest GitHub Actions
test-conda-python-3.11-pandas-nightly-numpy-nightly GitHub Actions
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly GitHub Actions
test-conda-python-3.11-spark-master GitHub Actions
test-conda-python-3.12 GitHub Actions
test-conda-python-3.12-cpython-debug GitHub Actions
test-conda-python-3.9 GitHub Actions
test-conda-python-3.9-pandas-1.1.3-numpy-1.19.5 GitHub Actions
test-conda-python-emscripten GitHub Actions
test-cuda-python-ubuntu-22.04-cuda-11.7.1 GitHub Actions
test-debian-12-python-3-amd64 GitHub Actions
test-debian-12-python-3-i386 GitHub Actions
test-fedora-39-python-3 GitHub Actions
test-ubuntu-22.04-python-3 GitHub Actions
test-ubuntu-22.04-python-313-freethreading GitHub Actions
test-ubuntu-24.04-python-3 GitHub Actions

@pitrou
Copy link
Member Author

pitrou commented Sep 19, 2024

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

@pitrou pitrou merged commit 0f7b5e5 into apache:main Sep 23, 2024
37 of 39 checks passed
@pitrou pitrou 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
Member

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

Looks good!

Copy link

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 free to 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