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

add closure that guarantees mutex unlock in loop #12980

Conversation

juanvallejo
Copy link
Contributor

Attempts to address #12930

Based on output from the linked test flake:

00:47:51.777 Running test/cmd/observe.sh:22: executing 'oc observe services --once --all-namespaces' expecting success and text 'default kubernetes'...
05:54:44.989 Connection to 172.18.7.33 closed by remote host.
05:54:44.993 Build step 'Execute shell' marked build as failure

It is a possibility that the test is hanging for multiple hours based on a deadlock happening. This patch eliminates that possibility.

cc @openshift/cli-review @enj @smarterclayton

@enj
Copy link
Contributor

enj commented Feb 15, 2017

The diff is significantly easier to see when ignoring whitespace.

@juanvallejo
Copy link
Contributor Author

[test]

@smarterclayton
Copy link
Contributor

Yeah the continue was broken. LGTM

@@ -431,6 +431,7 @@ func (o *ObserveOptions) Run() error {
go func() {
<-time.After(o.exitAfterPeriod)
lock.Lock()
defer lock.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary because we os.Exit(0). It doesn't hurt, so not an issue.

@smarterclayton
Copy link
Contributor

[merge]

@juanvallejo
Copy link
Contributor Author

flaked on #12923 re[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 8d7e9d6

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/265/) (Base Commit: 19aa349)

@fabianofranz
Copy link
Member

flaked on #12784 (comment) re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 8d7e9d6

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 18, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/320/) (Base Commit: aa49370) (Image: devenv-rhel7_5932)

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.

5 participants