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

Moved socket initialization to wroker processes if SO_REUSEPORT is available #241

Closed
wants to merge 1 commit into from

Conversation

dmatetelki
Copy link
Contributor

Many tests are failing.

@dmatetelki
Copy link
Contributor Author

Explained at #142

@dmatetelki
Copy link
Contributor Author

dmatetelki commented Jan 19, 2018

The tests fail because hitch_test.sh looks for hitch_hosts() with hitch_pid(). The parent process is not the process we look for now, but the children processes.

We would need to get the child process PIDs with: ps --ppid {hitch_pid} then iterate over the list with hitch_hosts().

@dmatetelki dmatetelki force-pushed the pr_144 branch 2 times, most recently from eef1ea3 to a20694a Compare January 19, 2018 12:37
@dmatetelki
Copy link
Contributor Author

I've assumed that on linux SO_REUSEPORT_WORKS is automatically enabled, but on the tavis machine that does not seem like to be the case.

@dmatetelki dmatetelki requested a review from dridi January 19, 2018 12:59
@dmatetelki dmatetelki force-pushed the pr_144 branch 3 times, most recently from 891a3e0 to 5674aef Compare January 23, 2018 15:16
…ailable

hitch_test.sh looks for worker processes for hitch hosts.
# Print the PIDs of the children of the daemon started with `start_hitch`

hitch_worker_pids() {
ps --ppid "$(hitch_pid)" | grep -Eo "^[ ]*[0-9]+"
Copy link

@ghost ghost Jan 31, 2018

Choose a reason for hiding this comment

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

OpenBSD's ps(1) doesn't know about --ppid, but pgrep(1) does via the short option -P, which behaves the same as FreeBSD's pgrep(1) and Debian's pgrep(1).

 pgrep -P "$(hitch_pid)"

is therefore portable and much simpler as it already lists child PIDs one per line.


hitch_hosts() {
SO_REUSEPORT_WORKS=false
if test "$(python -c 'import socket; print(1 if hasattr(socket, "SO_REUSEPORT") else 0)')" = "1"
Copy link

Choose a reason for hiding this comment

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

So this pulls in Python as test dependency, are you sure?
A shorter and simpler idiom would be

if python -c 'import socket; exit(not hasattr(socket, "SO_REUSEPORT"))'

or

if ! python -c 'import socket; exit(hasattr(socket, "SO_REUSEPORT"))'

depending on your taste.

@@ -252,6 +298,11 @@ curl_hitch() {
if ! $HAS_SPECIFIC_ARG
then
HITCH_HOST=$(hitch_hosts | sed 1q)
if [ -z $HITCH_HOST ];
Copy link

Choose a reason for hiding this comment

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

$HITCH_HOST should probably be quoted here.

@@ -318,6 +369,10 @@ s_client() {
if ! $HAS_CONNECT_OPT
then
HITCH_HOST=$(hitch_hosts | sed 1q)
if [ -z $HITCH_HOST ];
Copy link

Choose a reason for hiding this comment

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

Sames as above.

Copy link
Contributor Author

@dmatetelki dmatetelki Jan 31, 2018

Choose a reason for hiding this comment

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

Thank you for the comments, i'll adjust the commit.

I'm not sure about the new python dependency. I think all machines now have some python version and this one liner would run on all of them.

@dmatetelki
Copy link
Contributor Author

dmatetelki commented Jan 31, 2018

@daghf mentioned that this whole thing might not work with reload, i need to investigate.

@p-id p-id mentioned this pull request Sep 4, 2018
@daghf
Copy link
Member

daghf commented Oct 16, 2020

I'm working on a fix that supersedes this one. Closing

@daghf daghf closed this Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants