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

Prevent server termination with JSON and remove redundant code with error #1677

Merged

Conversation

davidBar-On
Copy link
Contributor

@davidBar-On davidBar-On commented Mar 27, 2024

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

  • Issues fixed (if any): Server side JSON problem #1631

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

Fix to two issues when JSON is used, one of them caused server termination:

  1. iperf_errexit() locked the mutex, but called iperf_json_finish() that also lock/unlock the mutex, so when the client terminated before the end of the test, the mutex unlock failed and the server terminated.
  2. Removed a redundant code, which was probably forgotten after copying it from some lines above. The redundant code caused redundant client error when it terminated.

@bmah888 bmah888 self-assigned this Apr 24, 2024
@bmah888 bmah888 added the bug:json Bugs related to JSON output label Apr 24, 2024
@bmah888
Copy link
Contributor

bmah888 commented Apr 24, 2024

Thanks for the PR! Initial tests with your patch are looking good so far, but I want to go back through the code (along with your commentary at the top of this PR) to understand how we (I) managed to mess this up without noticing. :-p

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 again for the PR! OK, I understand the original problems now. One change I think we need is to interchange the order of the mutex lock/unlock operations that you just moved/added. I think fixing these bugs uncovered some other problems (locking in the --json-stream codepaths and duplicated JSON output in the non --json-stream output codepath). I'm going to try to figure out fixes for these, not sure if I want to try to add to this PR or create a new PR for them.

src/iperf_api.c Show resolved Hide resolved
src/iperf_error.c Outdated Show resolved Hide resolved
src/iperf_error.c Outdated Show resolved Hide resolved
src/iperf_error.c Show resolved Hide resolved
@davidBar-On davidBar-On force-pushed the issue-1631-server-terminates-with-json branch from 1352df6 to 49c7aef Compare April 25, 2024 16:32
@davidBar-On
Copy link
Contributor Author

I switched between the lock and unlock, but if other related changes are needed, then I agree it is better to have a new PR (I don't have a problem with this).

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, OK I think this PR is good to go now! I'm hoping to have a fix for the other two problems shortly.

@bmah888 bmah888 merged commit 9632f93 into esnet:master Apr 25, 2024
3 checks passed
@bmah888 bmah888 mentioned this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:json Bugs related to JSON output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants