Skip to content

GH-49146: [C++] Add option to disable atfork handlers#49148

Merged
pitrou merged 1 commit into
apache:mainfrom
pitrou:gh49146-disable-atfork-handlers
Feb 17, 2026
Merged

GH-49146: [C++] Add option to disable atfork handlers#49148
pitrou merged 1 commit into
apache:mainfrom
pitrou:gh49146-disable-atfork-handlers

Conversation

@pitrou
Copy link
Copy Markdown
Member

@pitrou pitrou commented Feb 4, 2026

Rationale for this change

The atfork handlers we register in Arrow C++ are generally useful if the Arrow APIs are meant to be used from the child process, but they also have the unfortunate effect of executing non-async-signal-safe code in the child process even if Arrow is not be used there. That is not allowed by POSIX if the parent process is multi-threaded.

There are situations where fork() is only called just before exec(), and therefore it is not necessary to run any atfork handler.

What changes are included in this PR?

  1. Add a GetEnvVarInteger utility function to automate parsing of a numeric environment variable
  2. Remove hard-coded size limitations for environment variable values on Windows
  3. Add basic unit tests for our APIs for getting and setting environment variables
  4. Add an environment variable ARROW_REGISTER_ATFORK to disable the registration of atfork handlers at runtime

Are these changes tested?

The new environment variable cannot be easily tested automatically, so I've checked it manually.

Are there any user-facing changes?

No, only a new feature.

@pitrou pitrou force-pushed the gh49146-disable-atfork-handlers branch from 07d10a1 to a9c3279 Compare February 4, 2026 16:13
@pitrou pitrou force-pushed the gh49146-disable-atfork-handlers branch 2 times, most recently from 2753054 to 3a44920 Compare February 5, 2026 09:22
@pitrou pitrou force-pushed the gh49146-disable-atfork-handlers branch from 3a44920 to 57e9185 Compare February 5, 2026 10:21
@pitrou pitrou marked this pull request as ready for review February 5, 2026 10:26
@pitrou pitrou requested review from AlenkaF, raulcd and rok as code owners February 5, 2026 10:26
@pitrou pitrou requested review from WillAyd and adamreeve February 5, 2026 11:14
Copy link
Copy Markdown
Contributor

@adamreeve adamreeve left a comment

Choose a reason for hiding this comment

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

👍 This all looks correct and makes sense to me, thanks @pitrou


uint32_t res = GetEnvironmentVariableA(name.data(), value.data(),
static_cast<uint32_t>(value.size()));
if (res >= value.size()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This >= comparison was a bit confusing to me at first but this is fine as the == case should never happen.

@github-actions github-actions Bot added awaiting committer review Awaiting committer review awaiting review Awaiting review and removed awaiting review Awaiting review labels Feb 8, 2026
@pitrou pitrou merged commit ebaaf07 into apache:main Feb 17, 2026
57 of 58 checks passed
@pitrou pitrou removed awaiting review Awaiting review awaiting committer review Awaiting committer review labels Feb 17, 2026
@pitrou pitrou deleted the gh49146-disable-atfork-handlers branch February 17, 2026 16:23
@conbench-apache-arrow
Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

@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 ebaaf07.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 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.

2 participants