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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion cpp/src/arrow/filesystem/s3fs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3389,6 +3389,12 @@ struct AwsInstance {
ARROW_LOG(WARNING)
<< " arrow::fs::FinalizeS3 was not called even though S3 was initialized. "
"This could lead to a segmentation fault at exit";
// Leak the S3ClientFinalizer to avoid crashes when destroying remaining
// S3Client instances (GH-44071).
auto* leaked_shared_ptr =
new std::shared_ptr<S3ClientFinalizer>(GetClientFinalizer());
ARROW_UNUSED(leaked_shared_ptr);
return;
}
GetClientFinalizer()->Finalize();
#ifdef ARROW_S3_HAS_S3CLIENT_CONFIGURATION
Expand Down Expand Up @@ -3480,7 +3486,14 @@ Status EnsureS3Initialized() {
}

Status FinalizeS3() {
GetAwsInstance()->Finalize();
auto instance = GetAwsInstance();
// The AWS instance might already be destroyed in case FinalizeS3
// is called from an atexit handler (which is a bad idea anyway as the
// AWS SDK is not safe anymore to shutdown by this time). See GH-44071.
if (instance == nullptr) {
return Status::Invalid("FinalizeS3 called too late");
}
instance->Finalize();
return Status::OK();
}

Expand Down
39 changes: 39 additions & 0 deletions python/pyarrow/tests/test_fs.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@
import gzip
import os
import pathlib
from urllib.request import urlopen
import subprocess
import sys
import time

import pytest
import weakref
Expand All @@ -34,6 +36,10 @@
LocalFileSystem, SubTreeFileSystem, _MockFileSystem,
FileSystemHandler, PyFileSystem, FSSpecHandler,
copy_files)
from pyarrow.util import find_free_port


here = os.path.dirname(os.path.abspath(__file__))


class DummyHandler(FileSystemHandler):
Expand Down Expand Up @@ -2010,3 +2016,36 @@ def test_concurrent_s3fs_init():
finalize_s3()
"""
subprocess.check_call([sys.executable, "-c", code])


@pytest.mark.s3
def test_uwsgi_integration():
# GH-44071: using S3FileSystem under uwsgi shouldn't lead to a crash at shutdown
try:
subprocess.check_call(["uwsgi", "--version"])
except FileNotFoundError:
pytest.skip("uwsgi not installed on this Python")

port = find_free_port()
args = ["uwsgi", "-i", "--http", f"127.0.0.1:{port}",
"--wsgi-file", os.path.join(here, "wsgi_examples.py")]
proc = subprocess.Popen(args, stdin=subprocess.DEVNULL)
# Try to fetch URL, it should return 200 Ok...
try:
url = f"http://127.0.0.1:{port}/s3/"
start_time = time.time()
error = None
while time.time() < start_time + 5:
try:
with urlopen(url) as resp:
assert resp.status == 200
break
except OSError as e:
error = e
time.sleep(0.1)
else:
pytest.fail(f"Could not fetch {url!r}: {error}")
finally:
proc.terminate()
# ... and uwsgi should gracefully shutdown after it's been asked above
assert proc.wait() == 30 # UWSGI_END_CODE = 30
35 changes: 35 additions & 0 deletions python/pyarrow/tests/wsgi_examples.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

import pyarrow.fs


def application(env, start_response):
path = env['PATH_INFO']
members = path.split('/')
assert members[0] == ''
assert len(members) >= 2
root = members[1]
if root == 's3':
# See test_fs::test_uwsgi_integration
start_response('200 OK', [('Content-Type', 'text/html')])
# flake8: noqa
fs = pyarrow.fs.S3FileSystem()
return [b"Hello World\n"]
else:
start_response('404 Not Found', [('Content-Type', 'text/html')])
return [f"Path {path!r} not found\n".encode()]
1 change: 1 addition & 0 deletions python/requirements-test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ hypothesis
pandas
pytest
pytz
uwsgi; sys.platform != 'win32' and python_version < '3.13'
1 change: 1 addition & 0 deletions python/requirements-wheel-test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ hypothesis
pytest
pytz
tzdata; sys_platform == 'win32'
uwsgi; sys.platform != 'win32' and python_version < '3.13'

# We generally test with the oldest numpy version that supports a given Python
# version. However, there is no need to make this strictly the oldest version,
Expand Down
Loading