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

really fix netcat race #5803

Merged
merged 1 commit into from
Nov 30, 2018
Merged

really fix netcat race #5803

merged 1 commit into from
Nov 30, 2018

Conversation

Stebalien
Copy link
Member

We hit this once every few Jenkins runs. This:

  1. Ensures netcat has started before we try to use it.
  2. Waits for it to actually write the request before trying to read it.

License: MIT

@ghost ghost assigned Stebalien Nov 29, 2018
@ghost ghost added the status/in-progress In progress label Nov 29, 2018
@djdv
Copy link
Contributor

djdv commented Nov 29, 2018

rm -f nc_out nc_in && mkfifo nc_in nc_out
# 1. Abuse cat to buffer output.
# 2. Put cat in a subshell so we capture the PID of nc.
nc -k -l 5005 < nc_in > >(cat > nc_out) &
Copy link
Member

Choose a reason for hiding this comment

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

Sholudn't this be:

Suggested change
nc -k -l 5005 < nc_in > >(cat > nc_out) &
nc -k -l 5005 < <(cat nc_in) > nc_out &

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I needed to buffer output because netcat wasn't (or something like that...). Really, I'm not sure what was happening. I just know that everything locked up without this.

@Stebalien
Copy link
Member Author

This was still buggy because netcat wasn't flushing output and I couldn't find a way to convince netcat to do so (even with stdbuf, reading line by line, etc.).

This test now:

  1. Runs netcat in "single connection" mode and waits for it to exit by itself before checking the output.
  2. Uses the "verbose" mode to wait for netcat to start.

(I really hate buffering)

test/sharness/t0235-cli-request.sh Outdated Show resolved Hide resolved
test/sharness/t0236-cli-api-dns-resolve.sh Outdated Show resolved Hide resolved
@Stebalien Stebalien force-pushed the fix/2528-really branch 3 times, most recently from d39d20b to 0ed21dd Compare November 30, 2018 01:36
@Stebalien
Copy link
Member Author

Ok, I think I finally got it. We need to make sure to read the complete request before sending a complete response.

We hit this once every few Jenkins runs. This:

1. Ensures netcat has started before we try to use it.
2. Waits for it to actually write the request before trying to read it.

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@Stebalien
Copy link
Member Author

@magik6k this is ready for a final review. I think I finally fixed it... (a quick "let me just fixed this" turned into "let's learn all about fifos, glib buffering, bash + file descriptors, ...").

@Stebalien
Copy link
Member Author

(actually, this is a test fix so I'm just going to merge it to unblock #5801).

@Stebalien Stebalien merged commit 1dcac25 into master Nov 30, 2018
@ghost ghost removed the status/in-progress In progress label Nov 30, 2018
@gammazero gammazero deleted the fix/2528-really branch April 14, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants