Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid duplicate thread recycling. #1761

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Avoid duplicate thread recycling. #1761

merged 1 commit into from
Sep 20, 2024

Conversation

answer9030
Copy link
Contributor

@answer9030 answer9030 commented Sep 10, 2024

At the end of the test, the traffic thread has been reclaimed. If there is an exception in the control connection, it will cause the thread to be reclaimed repeatedly. Use sp->done to avoid repeated thread recycling.

PLEASE NOTE the following text from the iperf3 license. Submitting a
pull request to the iperf3 repository constitutes "[making]
Enhancements available...publicly":

You are under no obligation whatsoever to provide any bug fixes, patches, or
upgrades to the features, functionality or performance of the source code
("Enhancements") to anyone; however, if you choose to make your Enhancements
available either publicly, or directly to Lawrence Berkeley National
Laboratory, without imposing a separate written license agreement for such
Enhancements, then you hereby grant the following license: a non-exclusive,
royalty-free perpetual license to install, use, modify, prepare derivative
works, incorporate into other computer software, distribute, and sublicense
such enhancements or derivative works thereof, in binary and source code form.

The complete iperf3 license is available in the LICENSE file in the
top directory of the iperf3 source tree.

Copy link
Contributor

@bmah888 bmah888 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think the change makes sense...the only way that sp->done gets set to 1 would be if we're going to terminate the corresponding thread.

I'd probably just check for sp->done rather than a check for a specific value (sp->done == 1), this would be consistent with the other checks against sp->done.

There are two other places in iperf_client_api.c and one in iperf_server_api.c that do a similar iteration over all (or most) of the streams and try to pthread_cancel() their corresponding threads. Do you think that similar checks for sp->done would be appropriate in those cases as well?

@bmah888 bmah888 self-assigned this Sep 13, 2024
@bmah888 bmah888 added the bug label Sep 13, 2024
@answer9030
Copy link
Contributor Author

I think there is no need to perform similar checks on sp ->done in other places, as these places will not trigger duplicate thread recycling.

    At the end of the test, the traffic thread has been reclaimed.
If there is an exception in the control connection, it will cause
the thread to be reclaimed repeatedly.
Use sp->done to avoid repeated thread recycling.
@bmah888
Copy link
Contributor

bmah888 commented Sep 20, 2024

The original scenario for #1760 is a little difficult for me to reproduce exactly, but thanks for the detailed explanation of what triggers the segfault. I settled for tested this by more-or-less forcing the error handling on the client to run. I replaced this code in iperf_client_api.c (function iperf_run_client):

		if (iperf_set_send_state(test, TEST_END) != 0)
                    goto cleanup_and_fail;

with:

iperf_set_test_state(test, TEST_END);
//		if (iperf_set_send_state(test, TEST_END) != 0)
                    goto cleanup_and_fail;

I was able to reproduce the original segfault and as well as verify that the client exits sanely.

THanks again for the PR!

@bmah888 bmah888 merged commit 0ea9cb6 into esnet:master Sep 20, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants