Skip to content

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

Closed
westonpace wants to merge 2 commits into
apache:masterfrom
westonpace:bugfix/GH-15054--finalize-s3-at-test-end
Closed

GH-15054: [Python] Finalize S3 on pytest exit to prevent shutdown crashes#33793
westonpace wants to merge 2 commits into
apache:masterfrom
westonpace:bugfix/GH-15054--finalize-s3-at-test-end

Conversation

@westonpace

@westonpacewestonpace commented Jan 19, 2023

Copy link
Copy Markdown
Member

For some reason destroying an S3Client at shutdown can cause a crash in our tests. It appears to be because S3's logging is already shut down. I wouldn't think this would happen because I would think the linker would tear down S3 shared libraries AFTER it tears down Arrow shared libraries.

However, we can also finalize S3 in conftest.py to prevent this crash.

It would still be good to understand the root cause. Otherwise we need to advise our users to call finalize_s3.

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

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

@lidavidm

Copy link
Copy Markdown
Member

because I would think the linker would tear down S3 shared libraries AFTER it tears down Arrow shared libraries.

In a wheel though, we statically link everything, and then destruction order is undefined as usual?

doctest_namespace["local_path"] = str(tmp_path)
doctest_namespace["path"] = str(path)
yield

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the linter error is only because it expects two empty lines here.

Suggested change

@westonpace

Copy link
Copy Markdown
MemberAuthor

In a wheel though, we statically link everything, and then destruction order is undefined as usual?

Yes, I think that could explain it. Still, this fix would require pyarrow users to know about and remember calling the finalize S3 method. I'm going to pause this idea and make an attempt at an arrow::Shutdown.

@westonpace
westonpace marked this pull request as draft January 23, 2023 16:38
@westonpace

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #33858

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.

[CI][Python] wheel-manylinux2014-* sometimes crashed on pytest exit

3 participants

@westonpace@lidavidm@raulcd