-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 subthread signal handling #1752
Conversation
as mentioned in #1750 (comment), d312ddc in this PR cause client cannot exit in response to SIGTERM signal. |
Since multiple threads responding simultaneously to a signal leading to race condition, this is used to ensure that only the main thread handles the signal.
Thanks for the pull request! I'm looking this over. Also figuring out how to test this in my environment (the directions in #1750 were clear, I'm just not sure how to best do this with the resources I have available). Probably we'll want to squash and merge these two commits when we merge them to |
33a4b96 should fix it |
I tested this version, and it seems to work o.k.😄 For both the server and the client (using -P4) I tried to kill the main thread or a "worker" thread and in all cases termination was as expected. I guess that anyway it will also be good to also add a |
The logic for setting SIG_BLOCK in the two code segments is identical. How can we consolidate this into a single function so that both the client and server sides can call this function, making the code look more elegant? |
I apologize for the nitpicking to come: It would not have occurred to me to put in the conditional compilations in 33a4b96, because those signal definitions were at least as old as the POSIX.1-1990 standard. (Anecdotally I remember using them on BSD4 systems in the late 1980s.) I almost feel like they're not really needed, and removing them will make the source code a little easier to read and follow. Like if you try to build this on a system that for some reason doesn't have these signal definitions (such as I'm trying to test the code, but I don't really have a suitable environment set up to reproduce the original problem. It seems to work OK in what limited testing I can do though, so that's good. |
The conditional compilation was added just because the original signal capture function registration was defined like this. This is done simply to not break existing facilities and maintain consistency. For current Linux systems, I think da21dc2 is enough. |
Ah actually after a little research I see the other conditional compilation was originally added as cherry-pick of a part of PR #935. Let's keep 33a4b96 as a part of the PR. Thanks! |
If you can fix the nitpick I just posted about |
Thanks! Squash and merged. |
PLEASE NOTE the following text from the iperf3 license. Submitting a
pull request to the iperf3 repository constitutes "[making]
Enhancements available...publicly":
The complete iperf3 license is available in the
LICENSE
file in thetop directory of the iperf3 source tree.
Version of iperf3: master
Issues fixed (if any): Segmentation fault under running kernel's netfilter concurrency testing #1750
Brief description of code changes:
The main and sub-threads responding to the signal processing function at the same time will cause the sub-thread to access the test instance that has been released by the main thread, vice versa.