Skip to content

Avoid deadlocks after fork - #303

Open
danth wants to merge 3 commits into
eclipse-score:mainfrom
etas-contrib:avoid-deadlocks-after-fork
Open

Avoid deadlocks after fork#303
danth wants to merge 3 commits into
eclipse-score:mainfrom
etas-contrib:avoid-deadlocks-after-fork

Conversation

@danth

@danth danth commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Fork only copies the current thread, so from that point on we could deadlock if we access anything that was locked by another thread at the time of the fork.

According to man 2 fork we should only use async signal safe functions between fork and execve.

@danth
danth requested a deployment to workflow-approval July 9, 2026 12:31 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 9, 2026 12:31 — with GitHub Actions Waiting
@danth danth mentioned this pull request Jul 9, 2026
Open
2 tasks
@danth

This comment was marked as outdated.

Sign in to view
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from bcb913a to 2a70190 Compare July 10, 2026 08:09
@danth
danth requested a deployment to workflow-approval July 10, 2026 08:09 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 10, 2026 08:09 — with GitHub Actions Waiting
@github-actions

github-actions Bot commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //:license-check

Status: ⚠️ Needs Review

Click to expand output 
[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: cb0c03bc-50c8-45e3-b553-20bd625577e0
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Analyzing: target //:license-check (1 packages loaded)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)

Analyzing: target //:license-check (39 packages loaded, 10 targets configured)

Analyzing: target //:license-check (85 packages loaded, 10 targets configured)

Analyzing: target //:license-check (144 packages loaded, 2547 targets configured)

Analyzing: target //:license-check (156 packages loaded, 5498 targets configured)

Analyzing: target //:license-check (161 packages loaded, 5541 targets configured)

Analyzing: target //:license-check (161 packages loaded, 5547 targets configured)

Analyzing: target //:license-check (161 packages loaded, 5547 targets configured)

Analyzing: target //:license-check (164 packages loaded, 7434 targets configured)

Analyzing: target //:license-check (166 packages loaded, 10255 targets configured)

Analyzing: target //:license-check (168 packages loaded, 10283 targets configured)

Analyzing: target //:license-check (168 packages loaded, 10283 targets configured)

Analyzing: target //:license-check (168 packages loaded, 10283 targets configured)

INFO: Analyzed target //:license-check (169 packages loaded, 10409 targets configured).
[14 / 16] [Prepa] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar
[15 / 16] Building license.check.license_check.jar (); 0s disk-cache, multiplex-worker
INFO: Found 1 target...
Target //:license.check.license_check up-to-date:
  bazel-bin/license.check.license_check
  bazel-bin/license.check.license_check.jar
INFO: Elapsed time: 26.858s, Critical Path: 2.33s
INFO: 16 processes: 12 internal, 3 processwrapper-sandbox, 1 worker.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
usage: org.eclipse.dash.licenses.cli.Main [-batch <int>] [-cd <url>]
       [-confidence <int>] [-ef <url>] [-excludeSources <sources>] [-help] [-lic
       <url>] [-project <shortname>] [-repo <url>] [-review] [-summary <file>]
       [-timeout <seconds>] [-token <token>]

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown

The created documentation from the pull request is available at: docu-html

@danth
danth force-pushed the avoid-deadlocks-after-fork branch from 2a70190 to 9cc08d6 Compare July 10, 2026 08:22
@danth
danth requested a deployment to workflow-approval July 10, 2026 08:22 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 10, 2026 08:22 — with GitHub Actions Waiting
MaciejKaszynski
MaciejKaszynski requested changes Jul 14, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/configure_process.cpp Outdated
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/configure_process.cpp
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/configure_process.cpp Outdated
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/configure_process.cpp Outdated
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/process_group_manager.cpp
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from 9cc08d6 to 67c0d0f Compare July 14, 2026 09:57
@danth
danth requested a deployment to workflow-approval July 14, 2026 09:57 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 14, 2026 09:57 — with GitHub Actions Waiting
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from 67c0d0f to 9c28979 Compare July 14, 2026 10:05
@danth
danth requested a deployment to workflow-approval July 14, 2026 10:05 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 14, 2026 10:05 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 14, 2026 10:37 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 14, 2026 10:37 — with GitHub Actions Waiting
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from e249b48 to 4121230 Compare July 15, 2026 10:33
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from d0d9c81 to eec0536 Compare July 21, 2026 14:12
@danth
danth requested a deployment to workflow-approval July 21, 2026 14:13 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 21, 2026 14:13 — with GitHub Actions Waiting
@danth
danth requested a review from NicolasFussberger July 23, 2026 08:08
paulquiring
paulquiring reviewed Jul 24, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher.cpp Outdated
paulquiring
paulquiring reviewed Jul 24, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher.cpp Outdated
paulquiring
paulquiring reviewed Jul 24, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher.cpp Outdated
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from eec0536 to 6422d23 Compare July 24, 2026 08:31
@danth
danth requested a deployment to workflow-approval July 24, 2026 08:31 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 24, 2026 08:31 — with GitHub Actions Waiting
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from 6422d23 to 03a36bc Compare July 24, 2026 09:05
@danth
danth temporarily deployed to workflow-approval July 24, 2026 09:05 — with GitHub Actions Inactive
@danth
danth temporarily deployed to workflow-approval July 24, 2026 09:05 — with GitHub Actions Inactive
@danth

danth commented Jul 24, 2026

Copy link
Copy Markdown
Member Author

Tests failed due to a network error.

NicolasFussberger
NicolasFussberger reviewed Jul 27, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/configure_process.cpp Outdated
if (result == -1)
{
if (errno != EINTR)
_exit(EXIT_FAILURE);

@NicolasFussberger NicolasFussberger Jul 27, 2026

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.

I wonder if we should just return an error here and not directly exit.
Doing the exit in safe_log_and_exit / safe_log_errno_and_exit might be more obvious.

@danth danth Jul 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think it would be more "correct" but it also means we would have to add a lot of error checking which makes the later functions less clear. Since it's in a private namespace personally I would just leave it as is

NicolasFussberger
NicolasFussberger reviewed Jul 27, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/configure_process.cpp

void safe_log_errno_and_exit(std::initializer_list<std::string_view> parts)
{
for (auto part : parts)

@NicolasFussberger NicolasFussberger Jul 27, 2026

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.

Should we first assemble the message in a buffer, to avoid having this split across multiple lines?
The alternative would be we accept this in favour of keeping the code as simple as possible.

What do you think @danth @paulquiring ?

@danth danth Jul 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we should keep it simple

@danth
danth force-pushed the avoid-deadlocks-after-fork branch from 03a36bc to 8402f02 Compare July 27, 2026 08:43
@danth
danth requested a deployment to workflow-approval July 27, 2026 08:43 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 27, 2026 08:43 — with GitHub Actions Waiting
danth added 2 commits July 27, 2026 10:19
@danth
Log safely after forking a new process
f690ae9
@danth
Remove early validation of executable path
e66cd1f 
The executable could be incorrectly seen as missing if it is on a filesystem which has not been mounted yet. `execve` already fails if the executable does not exist so there is no need to check in advance.
@danth
danth force-pushed the avoid-deadlocks-after-fork branch from 8402f02 to e66cd1f Compare July 27, 2026 09:20
@danth
danth requested a deployment to workflow-approval July 27, 2026 09:20 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval July 27, 2026 09:20 — with GitHub Actions Waiting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NicolasFussberger NicolasFussberger NicolasFussberger left review comments
@MaciejKaszynski MaciejKaszynski MaciejKaszynski left review comments
+1 more reviewer
@paulquiring paulquiring paulquiring left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@danth danth

Labels

None yet

Projects

LCM - Lifecycle & Health FT
Status: Ready

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danth @paulquiring @anmittag @NicolasFussberger @MaciejKaszynski