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

connector-proxy: close stdout when child stdout closes #674

Closed
wants to merge 1 commit into from

Conversation

mdibaiee
Copy link
Member

@mdibaiee mdibaiee commented Sep 15, 2022

Description:

  • Here is an attempt to ensure that as soon as the child process exits, we close stdout of connector-proxy
  • However... this does not work. From my tests, I think the closing of the stdout is successful in connector-proxy, but my current understanding is that docker run does not propagate this closing of the stdout and keeps an open stdout even if the entrypoint closes its stdout. I tested with and without --init flag. Still investigating...

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

(anything that might help someone review this PR)


This change is Reviewable

@mdibaiee
Copy link
Member Author

mdibaiee commented Sep 15, 2022

Okay, I can confirm the issue is in behaviour of docker run, here is a rundown of the issue:

You can test the behaviour of docker run using a busybox container, to see how it handles stdin:

The example below does not print "test":

echo "test" | docker run busybox "head"

I tried attaching stdin, but still no "test":

echo "test" | docker run --attach stdin --attach stdout busybox "head"

By reading the the code, it seems that stdin is actually not opened at all unless --interactive is specified, and --interactive is problematic, because it "Keeps STDIN open even if not attached". In practice though, it seems to keep stdout open as well.

This prints "test":

echo "test" | docker run --interactive busybox "head"

I verified this PR by running a pull RPC using cdk, and then going inside the container. I can see that the code is correctly replacing stdout file descriptor of the process with /dev/null:

docker/connector $ cd /proc/1/fd
/proc/1/fd $ ls -lha
lr-x------ 1 nonroot nonroot 64 Sep 15 15:08 0 -> 'pipe:[35608139]'
lr-x------ 1 nonroot nonroot 64 Sep 15 15:08 1 -> /dev/null

However stdout of docker run stays open...

This is yet another reason for us to switch to using a network socket for communicating with the connector instead of stdio. cc @jgraettinger

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.

1 participant