Stop a test if one of the threads terminated because of an error

[davidBar-On]

• Version of iperf3 (or development branch, such as master or
  3.1-STABLE) to which this pull request applies:
  master

• Issues fixed (if any): Segfault during cleanup_server for bidirectional or with parallel stream tests ended early #1696

• Brief description of code changes (suitable for use as a commit message):

Suggested enhancement to terminate a test when one of the threads fail. Currently, even with one thread that fails, iperf3 continues to run the test (and reports 0 bytes transferred). The issue was detected while evaluating PR #1616.
UPDATE: Added a fix for #1696 - try to cancel a stream's thread only if it was created. This can happen if the client terminate before all threads for the streams where created.

The suggest fix approach is that both the client and the server will keep a counter for the number of threads running, which is shared by all threads. If a thread encounters an error, it subtract 1 from this counter before terminating. The client/server main loop is checking whether the counter value equals the expected number of threads.

My initially approach was using pthread_kill(thread, 0) to check whether any of the streams threads terminated. This is a more robust solution, since it also detects termination because of exceptions. However, I thought the overhead of such check is too high. I am not sure whether this is the case, since the check is done in the main thread.

[swlars]

Thanks for the pull request! This change looks interesting, but it requires a closer look and might take us a bit more time to look at it.

[MikeeI]

Hi, thanks for working on this.

I reproduced the underlying missing worker-to-main-loop propagation on current master@c9b7422, then replayed all four commits from this PR through d7ab071 onto that revision. The replay needed only one mechanical conflict resolution in the expanded iperf_error.c switch.

The fault injector keeps the control socket and second data stream live, selects the first data socket after the control connection, lets 31 write() calls succeed, then returns -1 with errno=EIO for that socket. I ran it against client and server sender workers with -t 4 -i 1 -P 2; the server case used -R.

The PR detects both worker failures, but I found these regressions:

1. The originating stream error is replaced. The worker correctly reports unable to write to stream socket: Input/output error, but the main loop overwrites IESTREAMWRITE/EIO with IEPTHREADNOTRUNNING. The client-facing result becomes a thread stopped running unexpectedly: , including a trailing colon without native error detail. The client exits 1 after about one second but emits the first interval twice before cleanup.

2. The server continues after destructive cleanup. On the server-worker injection, the client receives the generic SERVER_ERROR, while the server calls cleanup_server(test) and then continues its event loop. The observed tail was:

State set to SERVER_ERROR
All threads stopped
All threads stopped
select failed: Bad file descriptor

This replaces the root transfer error with a secondary select() failure and executes cleanup twice.

3. A partial pthread_create() failure now crashes. thread_number is assigned before pthread_create(), while cleanup treats thread_number > 0 as proof that the thread exists. Faulting the second creation with EAGAIN made the reconstructed PR exit 139. Current master under the same injector cleanly reports unable to create thread and exits 1. Keeping thread_created separate from diagnostic numbering avoids canceling or joining an uncreated thread.

4. Normal reverse teardown emits false worker failures. Successful persistent-server forward/reverse runs with both -P 2 and -P 1 returned 0, but each reverse teardown logged errors such as:

Server Worker Thread 1 FD 5 failed - unable to write to stream socket: Bad file descriptor

The worker is classifying an expected teardown race as a transfer failure.

There are two related ownership problems in the current approach:

• running_threads is role-global static volatile, not test-owned. Workers modify it under running_mutex, but main loops read it without that mutex; volatile does not provide synchronization.
• Workers call iperf_err() directly, moving text, JSON-stream, logfile, and callback output into worker-thread context. In JSON streaming, the observed sequence was start, interval, interval, error, end; full JSON contained the generic error, two intervals, and an end object.

The reconstructed candidate built successfully, make -s check passed 5/5, and test_commands.sh 127.0.0.1 exited 0. Those checks did not catch the partial-creation crash or the timing-sensitive reverse teardown diagnostics.

Would it make sense to keep worker completion as test- or stream-owned synchronized state containing the first exact i_errno and native errno, retain thread_created as independent lifecycle truth, and let only the owning main loop report the error and enter one cleanup path followed by an immediate return?

I checked the existing PR body and discussion; these current-master reproduction results and regressions were not already reported.

Disclosure

Investigated thoroughly with GPT-5.6 (extra high reasoning effort), using Oh My Pi as the agent framework.

This report is not generic or unreviewed AI-generated output. Its claims were checked against the cited evidence, and it includes the relevant detail intended to help maintainers resolve the issue.

If reports like this are not useful to the project, please let me know and I will refrain from submitting similar ones. My intent is to help without wasting maintainer time or energy or discouraging their work.

Thank you for your work.

[davidBar-On]

@MikeeI thanks for this input. At least for me, as the PR author, it is helpful (I don't know about the iperf3 project in general).

Regarding the comments / suggested changes, I implemented some (see for details below). I would appreciate it if you can run this evaluation again on the new commit (assuming it does not use too many tokens ...).

One more point that your AI may help with. Although per your results it seems that in general this PR is working well, it disturbs me that the main thread has to "count" the active worker threads, and that it cannot receive a signal directly about failure of one of these threads. If there is a way to do it (which portable in the main OSs) I would be happy to know how.

Regarding the suggested changes:

1. The originating stream error is replaced. ...

It will take a lot of testing to make sure that setting the error is required only if it is 0. I prefer not to touch that as it seems to be a minor issue. (May be re-considered after the PR is merged.)

2. The server continues after destructive cleanup. ...

I prefer to keep the double cleanup than risking a case were no cleanup will be preformed.

3. A partial pthread_create() failure now crashes. ...

Should be fixed now. Moved setting the thread counter after the thread was successful created.

4. Normal reverse teardown emits false worker failures. ...

I don't understand this comment ... If it means that the server does not reset the i_error/errno, then this issue is already handled by PR #2052 and I will not add it here.

running_threads is role-global static volatile, not test-owned. Workers modify it under running_mutex, but main loops read it without that mutex; volatile does not provide synchronization.

While the comment is correct, practically I don't think it is an issue, as the thread counter increases only during the threads creation. Therefore, for simplicity, I won't add the mutex in the main thread.

Workers call iperf_err() directly, moving text, JSON-stream, logfile, and callback output into worker-thread context. ...

This is indeed an issue that I overlooked. I now changed the code so the workers errors will only be printed to sdtoutand will not be added to the JSON output. (Adding them to the JSON output, if needed at all, is too complex to be included in this PR.)