Skip to content

client: do not fork a child process unless a tunnel is configured - #1604

Open
bpalermo wants to merge 4 commits into
envoyproxy:mainfrom
bpalermo:up/no-fork-without-tunnel
Open

bpalermo wants to merge 4 commits into
envoyproxy:mainfrom
bpalermo:up/no-fork-without-tunnel

Conversation

@bpalermo

@bpalermo bpalermo commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Description

This PR is related to #1609

ProcessImpl::run() always goes through EncapsulationSubProcessRunner, which fork()s a child even when no --tunnel-uri is configured. In that case the child does nothing but sem_post() a process-shared semaphore and _exit(), while the parent blocks in sem_wait() until it does. The parent is already multithreaded at that point (at least the SignalHandler thread; in -c opt builds also the allocator's state), so the child can deadlock on a lock owned by a thread that does not exist in it before it ever reaches sem_post(), and the client hangs at startup without sending a request. This runs the load in-process unless a tunnel is actually requested; the fork is still used for the tunneling path, where the child runs the encapsulating Envoy.

Notes for Reviewers

  • The runner's terminate/cancel paths already null-check encap_runner_, so no other change is needed.
  • Testing: //test:process_test and //test:python_test (which includes the tunneling integration tests) pass. Version history updated.

ProcessImpl::run() always went through EncapsulationSubProcessRunner,
forking a child even without --tunnel-uri; in that case the child only
posts a process-shared semaphore and exits while the parent blocks in
sem_wait(). The parent is already multithreaded at that point (the
signal handler thread, allocator state), so the child could deadlock on
a lock owned by a thread that does not exist in it before ever reaching
sem_post(), leaving nighthawk_client hung at startup with no request
sent. Run the load in-process unless a tunnel is actually requested.

Signed-off-by: Bruno Palermo <bruno.palermo@superbid.net>
Brings the branch up to date with the Envoy bump to 82c182e, the
envoy_package additions, envoyproxy#1598 and envoyproxy#1599. The only conflict was the
version history changelist, where both sides add a bullet; both are
kept.

Signed-off-by: Bruno Palermo <bruno.palermo@superbid.net>

@eric846 eric846 left a comment

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 seemed to fail the tsan test in the CI, in a Python integration test.

Could you see if you can reproduce any failure like that locally with ci/do_ci.sh tsan? It should also fail on your machine if there's a real issue.

It's also possible for the CI to be flaky, like if the VM used in the CI is slow. In that case we would have to look further into the test and possibly adjust some numbers to reduce the flakiness.

Brings the branch up to date after envoyproxy#1601 landed. The only conflict was
the version history changelist, where both sides add a bullet; both are
kept. Pushing this also gives the tsan job a second run: the failure
eric846 saw was test_https_prefetching, which asserts 50 TLS connections
are established inside a 1 second run, and it failed because the counter
was absent rather than wrong, i.e. no connection completed in time.

Signed-off-by: Bruno Palermo <bruno.palermo@superbid.net>
No code change. The tsan job has failed in a different wall-clock
sensitive Python integration test on each run (test_https_prefetching,
then test_linear_ramping_rate_limiter_plugin), and upstream main failed
test_tracing_zipkin on a Dockerfile base image bump, so this samples the
job once more.

Signed-off-by: Bruno Palermo <bruno.palermo@superbid.net>
@bpalermo

Copy link
Copy Markdown
Contributor Author

@eric846 indeed seems it's a flaky test.

@bpalermo
bpalermo requested a review from eric846 September 17, 2026 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants