GH-15054: [C++] Change s3 finalization to happen after arrow threads finished, add pyarrow exit hook #33858

westonpace · 2023-01-25T01:04:23Z

CRITICAL FIX: When statically linking error with AWS it was possible to have a crash on shutdown/exit. Now that should no longer be possible.

BREAKING CHANGE: S3 can only be initialized and finalized once.

BREAKING CHANGE: S3 (the AWS SDK) will not be finalized until after all CPU & I/O threads are finished.

Closes: [CI][Python] wheel-manylinux2014-* sometimes crashed on pytest exit #15054

github-actions · 2023-01-25T01:04:46Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

vibhatha · 2023-01-27T01:10:36Z

cpp/src/arrow/filesystem/s3fs.cc

+    switch (options.log_level) {
+      LOG_LEVEL_CASE(Fatal)
+      LOG_LEVEL_CASE(Error)
+      LOG_LEVEL_CASE(Warn)
+      LOG_LEVEL_CASE(Info)
+      LOG_LEVEL_CASE(Debug)
+      LOG_LEVEL_CASE(Trace)
+      default:
+        aws_log_level = Aws::Utils::Logging::LogLevel::Off;
+    }


is the new tab an expected change?

Yes. The initialize and finalize methods were file-scoped methods previously. Now they have become the constructor/destructor of the AwsInstance class. Since they are in a class they get some indentation.

github-actions · 2023-03-27T15:48:33Z

Closes: [CI][Python] wheel-manylinux2014-* sometimes crashed on pytest exit #15054

jorisvandenbossche · 2023-04-06T12:37:36Z

@westonpace do we want to try to get this into 12.0.0? (don't know the state of the PR though)

westonpace · 2023-04-11T15:59:52Z

@jorisvandenbossche Yes, I think we do. I'll rebase this right now but I think the main concern here was just getting a review / second opinion. I'm going to label it a blocker.

zeroshade

I don't know if I'm the best to review here, but at least at the moment I don't see anything glaringly problematic with this and it seems like a good idea in general.

It feels like it should be better tested, but I'm not particularly sure how would be a good way to do so. But that might just be because it's the end of the day lol.

felipecrv · 2023-04-13T01:42:03Z

cpp/src/arrow/filesystem/s3fs.cc

 }

+bool IsAwsInitialized() { return !!GetAwsInstance({}); }


If this gets called before InitializeS3, the options passed there will be ignored and an instance based on {} options will stick around. I think bringing back the bool flag can avoid these risks.

westonpace · 2023-04-13T16:23:07Z

Thanks @felipecrv , that is a good point regarding IsInitialized. I added the flag back but put it inside the instance object. I put everything inside the instance object and I think the code is simpler now. What do you think of this version?

felipecrv

This LGTM. Thank you!

westonpace · 2023-04-14T17:02:03Z

These R crashes are a little concerning since they seem to be crashes at shutdown. Maybe it is related.

westonpace · 2023-04-14T17:41:34Z

Yes, the R crashes are relevant. With this fix we ensure that AWS::ShutdownAPI is the last call we make to AWS. However, AWS has static global objects of its own. So it is entirely possible that these have been destroyed by the time AWS::ShutdownAPI is called. In fact, by prolonging the call to ShutdownAPI here it seems we are making the R situation worse because it is more likely these other static globals have been destroyed. There is an open AWS issue for this at aws/aws-sdk-cpp#1550 . However, it does not sound like it will be resolved quickly. In the meantime an R atexit hook of some kind will be required (technically it has always been required but we've gotten lucky).

paleolimbot · 2023-04-14T19:28:02Z

r/R/arrow-package.R

+  # Registers a callback to run at session exit
+  .onLoad <- function(libname, pkgname) {
+    print(parent.env(environment()))
+    reg.finalizer(parent.env(environment()), finalize_s3, onexit=TRUE)
+  }
+


I know you asked about this earlier and found .onUnload()...just putting a note here in case this gets forgotten about.

If .onUnload() isn't working you can create your own environment an register a finalizer on that:

s3_finalizer <- new.env(parent = emptyenv()) .onLoad <- function(...) { # ... reg.finalizer(s3_finalizer, finalize_s3, onexit=TRUE) }

You probably shouldn't register a finalizer on the package namespace because somebody else might have done that (maybe even R).

There is also .onLoad() defined twice here as written (you should be able to use the existing .onLoad() to register this hook if you can't in fact use .onUnload().

Ah, ok. I will change to this pattern instead of parent.env(environment())

Also, I just realized I left the print in >_<

And if .onUnload() really isn't working make sure to put a comment in explaining that (so I don't forget about this and try to consolidate our namespace unload incorrectly)

Ok, I think I've resolved all your concerns now. Thanks for looking at this.

And if .onUnload() really isn't working make sure to put a comment in explaining that (so I don't forget about this and try to consolidate our namespace unload incorrectly)

Added a comment

westonpace · 2023-04-14T19:28:54Z

The broken C++ tests are unrelated (will be fixed by #35145). It seems the R tests are passing now. Would appreciate a review from @paleolimbot or @thisisnic on the R changes.

@kou do you know if cglib / ruby is using S3? If so, is it calling FinalizeS3 in some kind of shutdown hook? Or do we need a change there too.

kou · 2023-04-14T21:45:54Z

do you know if cglib / ruby is using S3? If so, is it calling FinalizeS3 in some kind of shutdown hook? Or do we need a change there too.

Yes, I know. GLib/Ruby is using S3.
It doesn't call FinalizeS3() automatically but it provides APIs that can call InitializeS3() and FinalizeS3() explicitly. So we don't need changes for GLib/Ruby here for now.
If a report from community is raised, we can consider this.

westonpace · 2023-04-14T22:04:03Z

Thanks, that sounds reasonable. I'll merge when CI passes then.

…finished, add pyarrow exit hook (#33858) CRITICAL FIX: When statically linking error with AWS it was possible to have a crash on shutdown/exit. Now that should no longer be possible. BREAKING CHANGE: S3 can only be initialized and finalized once. BREAKING CHANGE: S3 (the AWS SDK) will not be finalized until after all CPU & I/O threads are finished. * Closes: #15054 Authored-by: Weston Pace <[email protected]> Signed-off-by: Weston Pace <[email protected]>

assignUser · 2023-04-17T15:24:55Z

I have added the appropriate labels for visibility

ursabot · 2023-04-17T22:52:45Z

Benchmark runs are scheduled for baseline = 9626c7d and contender = 1de159d. 1de159d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Finished ⬇️0.51% ⬆️0.51%] ursa-i9-9960x
[Finished ⬇️3.42% ⬆️0.89%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 1de159d0 ec2-t3-xlarge-us-east-2
[Failed] 1de159d0 test-mac-arm
[Finished] 1de159d0 ursa-i9-9960x
[Finished] 1de159d0 ursa-thinkcentre-m75q
[Finished] 9626c7d5 ec2-t3-xlarge-us-east-2
[Failed] 9626c7d5 test-mac-arm
[Finished] 9626c7d5 ursa-i9-9960x
[Finished] 9626c7d5 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

…reads finished, add pyarrow exit hook (apache#33858) CRITICAL FIX: When statically linking error with AWS it was possible to have a crash on shutdown/exit. Now that should no longer be possible. BREAKING CHANGE: S3 can only be initialized and finalized once. BREAKING CHANGE: S3 (the AWS SDK) will not be finalized until after all CPU & I/O threads are finished. * Closes: apache#15054 Authored-by: Weston Pace <[email protected]> Signed-off-by: Weston Pace <[email protected]>

westonpace requested a review from AlenkaF as a code owner January 25, 2023 01:04

github-actions bot added Component: C++ Component: Python labels Jan 25, 2023

westonpace mentioned this pull request Jan 25, 2023

GH-15054: [Python] Finalize S3 on pytest exit to prevent shutdown crashes #33793

Closed

vibhatha reviewed Jan 27, 2023

View reviewed changes

westonpace force-pushed the bugfix/GH-15054--s3-crash-on-exit branch from ac63d43 to c6ab7cb Compare February 4, 2023 13:24

amol- changed the title ~~[C++] GH-15054: Change s3 finalization to happen after arrow threads finished, add pyarrow exit hook~~ GH-15054: [C++] Change s3 finalization to happen after arrow threads finished, add pyarrow exit hook Mar 27, 2023

westonpace force-pushed the bugfix/GH-15054--s3-crash-on-exit branch from c6ab7cb to 95b1458 Compare April 11, 2023 16:04

github-actions bot added the awaiting review Awaiting review label Apr 11, 2023

zeroshade approved these changes Apr 12, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Apr 12, 2023

felipecrv reviewed Apr 13, 2023

View reviewed changes

felipecrv approved these changes Apr 13, 2023

View reviewed changes

westonpace requested review from paleolimbot and thisisnic as code owners April 14, 2023 18:48

github-actions bot added the Component: R label Apr 14, 2023

paleolimbot reviewed Apr 14, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting merge Awaiting merge awaiting changes Awaiting changes labels Apr 14, 2023

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 14, 2023

R lint fix

1780990

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 14, 2023

Added a warning that should help users remember to call Finalize

f817dfe

westonpace merged commit 1de159d into apache:main Apr 15, 2023

westonpace added this to the 12.0.0 milestone Apr 15, 2023

assignUser added Breaking Change Includes a breaking change to the API Critical Fix Bugfixes for security vulnerabilities, crashes, or invalid data. labels Apr 17, 2023

acezen mentioned this pull request Jun 9, 2023

[Bug]: Loading from S3 bucket got segment fault error apache/incubator-graphar#187

Closed

1 task

This was referenced Jun 23, 2023

Crash on exist with pyarrow >= 12.0.0 NOAA-OWP/t-route#614

Closed

Pin pyarrow to avoid crash in 12.x NOAA-OWP/ngen#542

Merged

This was referenced Jun 28, 2023

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

Closed

GH-36346: [C++][S3] Shutdown aws-sdk-cpp related resources on finalize #36437

Closed

mapleFU mentioned this pull request Jul 23, 2023

[Java] while using s3 FileSystemDatasetFactory getting this exception #36069

Closed

robertmaynard mentioned this pull request Aug 1, 2023

[C++] S3 Finalization functions need to exist even when S3 isn't enabled. #36974

Closed

jorisvandenbossche mentioned this pull request Oct 20, 2023

[Python] Flaky Segfault on Python Interpreter Exit #38352

Closed

felipecrv mentioned this pull request Oct 24, 2023

[Python] Stop initializing s3 upon import #38364

Closed

jepett0 mentioned this pull request Jul 2, 2024

Fix memory leak issue with AWS API usage ydb-platform/ydb#6160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-15054: [C++] Change s3 finalization to happen after arrow threads finished, add pyarrow exit hook #33858

GH-15054: [C++] Change s3 finalization to happen after arrow threads finished, add pyarrow exit hook #33858

westonpace commented Jan 25, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Jan 25, 2023

vibhatha Jan 27, 2023

westonpace Jan 27, 2023

github-actions bot commented Mar 27, 2023

jorisvandenbossche commented Apr 6, 2023

westonpace commented Apr 11, 2023

zeroshade left a comment

felipecrv Apr 13, 2023

westonpace commented Apr 13, 2023

felipecrv left a comment

westonpace commented Apr 14, 2023

westonpace commented Apr 14, 2023

paleolimbot Apr 14, 2023

paleolimbot Apr 14, 2023

westonpace Apr 14, 2023

westonpace Apr 14, 2023

paleolimbot Apr 14, 2023

westonpace Apr 14, 2023

westonpace Apr 14, 2023

westonpace commented Apr 14, 2023

kou commented Apr 14, 2023

westonpace commented Apr 14, 2023

assignUser commented Apr 17, 2023

ursabot commented Apr 17, 2023

GH-15054: [C++] Change s3 finalization to happen after arrow threads finished, add pyarrow exit hook #33858

GH-15054: [C++] Change s3 finalization to happen after arrow threads finished, add pyarrow exit hook #33858

Conversation

westonpace commented Jan 25, 2023 • edited by github-actions bot Loading

github-actions bot commented Jan 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 27, 2023

jorisvandenbossche commented Apr 6, 2023

westonpace commented Apr 11, 2023

zeroshade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonpace commented Apr 13, 2023

felipecrv left a comment

Choose a reason for hiding this comment

westonpace commented Apr 14, 2023

westonpace commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonpace commented Apr 14, 2023

kou commented Apr 14, 2023

westonpace commented Apr 14, 2023

assignUser commented Apr 17, 2023

ursabot commented Apr 17, 2023

westonpace commented Jan 25, 2023 •

edited by github-actions bot

Loading