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

UPSTREAM: 16590: Create all streams before copying in exec/attach #5585

Merged
merged 1 commit into from
Nov 3, 2015

Conversation

ncdc
Copy link
Contributor

@ncdc ncdc commented Nov 2, 2015

Create error, stdin, stdout, stderr streams first, and only start
copying once all the streams have been created. This fixes an issue
where the client immediately starts sending data for stdin before all
the other streams have been created. This ends up blocking the spdy
connection frame handler and causes the entire exec/attach session to
time out.

Fixes #5377

@ncdc
Copy link
Contributor Author

ncdc commented Nov 2, 2015

@ncdc
Copy link
Contributor Author

ncdc commented Nov 2, 2015

[test]

@csrwng
Copy link
Contributor

csrwng commented Nov 2, 2015

LGTM

@smarterclayton smarterclayton added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2015
@csrwng
Copy link
Contributor

csrwng commented Nov 2, 2015

sorry... I'm taking back my LGTM... seeing a timeout in at least one windows box

@csrwng
Copy link
Contributor

csrwng commented Nov 2, 2015

@ncdc I'm seeing timeouts in windows with this code... not sure what is different since I tried your patch, seems like the same change to me. I've tried in Windows Server 2012 R2 (from AWS), and Windows 7 Pro (vagrant on virtualbox)

https://gist.github.com/csrwng/55f69dacc11f356a4c4a

@ncdc
Copy link
Contributor Author

ncdc commented Nov 2, 2015

I've identified the issue. I'm working on a fix. This may affect upstream too.

Create error, stdin, stdout, stderr streams first, and only start
copying once all the streams have been created. This fixes an issue
where the client immediately starts sending data for stdin before all
the other streams have been created. This ends up blocking the spdy
connection frame handler and causes the entire exec/attach session to
time out.
@ncdc
Copy link
Contributor Author

ncdc commented Nov 2, 2015

Fix applied to upstream and here

@smarterclayton
Copy link
Contributor

What was the change?

@liggitt
Copy link
Contributor

liggitt commented Nov 2, 2015

if e.stderr != nil && !e.tty {

@ncdc
Copy link
Contributor Author

ncdc commented Nov 2, 2015

What Jordan said. I missed copying a condition from the old code to the new code correctly. I left out the tty part, so the client was always sending stderr even with tty=true.

@liggitt
Copy link
Contributor

liggitt commented Nov 2, 2015

LGTM, @csrwng can you test?

@csrwng
Copy link
Contributor

csrwng commented Nov 2, 2015

Latest test in Windows 2012R2 connecting to an Origin server with current master version is good. Ship it!

@smarterclayton
Copy link
Contributor

[test]

On Mon, Nov 2, 2015 at 3:33 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6602/
)


Reply to this email directly or view it on GitHub
#5585 (comment).

@ncdc
Copy link
Contributor Author

ncdc commented Nov 2, 2015

job 6617 failure was the github.com connectivity issue

[test]

@smarterclayton
Copy link
Contributor

[test]

On Mon, Nov 2, 2015 at 5:57 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6624/
)


Reply to this email directly or view it on GitHub
#5585 (comment).

@liggitt
Copy link
Contributor

liggitt commented Nov 3, 2015

rsh forbidden flake (which is fixed), re[test]

@ncdc
Copy link
Contributor Author

ncdc commented Nov 3, 2015

[test]

@ncdc
Copy link
Contributor Author

ncdc commented Nov 3, 2015

Github timeout

[test]

On Tuesday, November 3, 2015, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6705/
)


Reply to this email directly or view it on GitHub
#5585 (comment).

@ncdc
Copy link
Contributor Author

ncdc commented Nov 3, 2015

[test]

On Tuesday, November 3, 2015, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6708/
)


Reply to this email directly or view it on GitHub
#5585 (comment).

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 10a9b21

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6711/)

@ncdc
Copy link
Contributor Author

ncdc commented Nov 3, 2015

Yes, green Jenkins!!!

@ncdc
Copy link
Contributor Author

ncdc commented Nov 3, 2015

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6711/) (Image: devenv-rhel7_2637)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 10a9b21

openshift-bot pushed a commit that referenced this pull request Nov 3, 2015
@openshift-bot openshift-bot merged commit 192bef7 into openshift:master Nov 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exec will hang in windows when streaming in 2MB or larger files into stdin
5 participants