Skip to content

Comments

Fix TestBuildCancelationKillsSleep to not fail on Windows#12109

Merged
LK4D4 merged 1 commit intomoby:masterfrom
runcom:11625-fix-windows-test-build-cancellation
Apr 9, 2015
Merged

Fix TestBuildCancelationKillsSleep to not fail on Windows#12109
LK4D4 merged 1 commit intomoby:masterfrom
runcom:11625-fix-windows-test-build-cancellation

Conversation

@runcom
Copy link
Member

@runcom runcom commented Apr 6, 2015

Fixes: #11625

Signed-off-by: Antonio Murdaca [email protected]

duglin
duglin reviewed Apr 6, 2015
View reviewed changes
integration-cli/docker_cli_build_test.go Outdated
Copy link
Contributor

@duglin duglin Apr 6, 2015

Choose a reason for hiding this comment

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

If this works, you should just remove this line

Copy link
Member Author

@runcom runcom Apr 6, 2015

Choose a reason for hiding this comment

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

@duglin yes I just pushed to see what's happening on Windows, I'm not sure it works btw

@runcom runcom force-pushed the 11625-fix-windows-test-build-cancellation branch from 472426a to bfce1f4 Compare April 6, 2015 15:21
@runcom
Copy link
Member Author

runcom commented Apr 6, 2015

test isn't racy anymore. It passed on janky with the daemonTime fix, but yet on Windows even if the order of execution is now respected it still fails with:

--- FAIL: TestBuildCancelationKillsSleep (0.44s)
    docker_cli_build_test.go:2074: wait failed during build run: *exec.ExitError exit status 1
    docker_cli_build_test.go:2021: docker events had bad exit status: exit status 1

Definitively something wrong with process killing on Windows (?) ping @jfrazelle

@runcom runcom force-pushed the 11625-fix-windows-test-build-cancellation branch from bfce1f4 to 1298033 Compare April 6, 2015 15:42
@runcom runcom force-pushed the 11625-fix-windows-test-build-cancellation branch 5 times, most recently from fc9c2cd to d3fcc45 Compare April 6, 2015 16:41
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 6, 2015

@runcom Hmm, error seems legit for me. You're killing events, so it exits with -1. WDYT about using until for streaming autoexit? Or you can try send SIGTERM.

@runcom
Copy link
Member Author

runcom commented Apr 6, 2015

@LK4D4 that's ok but why the same test goes well on janky, I just don't understand this.. Also !IsKilled(err) is failing there

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 6, 2015

@runcom Maybe windows cmd has different signals and all stuff.
And it is indeed.
http://golang.org/src/syscall/syscall_windows.go?#L807

@pwaller
Copy link
Contributor

pwaller commented Apr 6, 2015

@LK4D4 that's ok but why the same test goes well on janky, I just
don't understand this.. Also I don't know why who wrote the test in the
beginning was looking to check !IsKilled(err)

The idea of the test was to check in as many ways as possible that the
reason for the death was because we signalled it.

pwaller
pwaller reviewed Apr 6, 2015
View reviewed changes
integration-cli/docker_cli_build_test.go
Copy link
Contributor

@pwaller pwaller Apr 6, 2015

Choose a reason for hiding this comment

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

I would be interested to hear the reasoning behind removing the waitgroup. My reasoning behind having it was to ensure that if the goroutine did not close for some reason due to future modifications to the code, it would block, and goroutine leakage could be detected. I use this pattern in this manner to specify that I really want this goroutine not to last longer than the calling scope, and if it does it should be detected by someone, because the caller will block at that wait.

var wg sync.WaitGroup
defer wg.Wait()
wg.Add(1)
go func() { 
  defer wg.Done()
  ...
}()

Copy link
Member Author

@runcom runcom Apr 6, 2015

Choose a reason for hiding this comment

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

Haven't thought about that, I thought was to better synchronize the goroutine with the external context and thought that wouldn't be needed anymore. Cool, I'll add that back then.

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 6, 2015

@pwaller It failing on windows, because Signaled always return false on windows I think

@runcom runcom force-pushed the 11625-fix-windows-test-build-cancellation branch 2 times, most recently from a063e4e to db734d1 Compare April 6, 2015 17:50
@runcom
Copy link
Member Author

runcom commented Apr 6, 2015

would just status.Signal() == os.Kill detect we killed the buildCmd and since os.Kill works on Windows too the test will pass? NO

@runcom runcom force-pushed the 11625-fix-windows-test-build-cancellation branch 3 times, most recently from b76021c to c971e01 Compare April 6, 2015 18:42
LK4D4
LK4D4 reviewed Apr 6, 2015
View reviewed changes
integration-cli/docker_cli_build_test.go Outdated
Copy link
Contributor

@LK4D4 LK4D4 Apr 6, 2015

Choose a reason for hiding this comment

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

Look at #12125

Copy link
Member Author

@runcom runcom Apr 6, 2015

Choose a reason for hiding this comment

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

Roger that

@runcom runcom force-pushed the 11625-fix-windows-test-build-cancellation branch 2 times, most recently from e7a4c08 to 033e921 Compare April 7, 2015 18:28
@runcom runcom force-pushed the 11625-fix-windows-test-build-cancellation branch from 033e921 to a55c1fb Compare April 7, 2015 18:56
@runcom
Copy link
Member Author

runcom commented Apr 7, 2015

@LK4D4 @pwaller Since both WaitStatus.Signal() and WaitStatus.Signaled() aren't intentionally implemented on Windows I added a condition to check whether the ExitStatus() is not equal to zero or it is..In tests, and especially this, we do kill the process so we can be relatively sure the bad exit status is because we killed it. The other option would be not to run this test on windows (as it was before)

@runcom runcom force-pushed the 11625-fix-windows-test-build-cancellation branch from a55c1fb to 816cb13 Compare April 7, 2015 19:03
@runcom runcom mentioned this pull request Apr 7, 2015
Closed
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 7, 2015

@runcom I think it is better to run it on windows.

@runcom
Copy link
Member Author

runcom commented Apr 7, 2015

EDIT: seems ok now :) @LK4D4

@runcom runcom force-pushed the 11625-fix-windows-test-build-cancellation branch from 816cb13 to 078e497 Compare April 7, 2015 20:30
@jessfraz
Copy link
Contributor

jessfraz commented Apr 9, 2015

sorrrry rebase :(

@runcom runcom force-pushed the 11625-fix-windows-test-build-cancellation branch from 078e497 to d38c901 Compare April 9, 2015 19:03
@runcom
Copy link
Member Author

runcom commented Apr 9, 2015

@jfrazelle done

@jessfraz
Copy link
Contributor

jessfraz commented Apr 9, 2015

LGTM

1 similar comment
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 9, 2015

LGTM

LK4D4 added a commit that referenced this pull request Apr 9, 2015
@LK4D4
Merge pull request #12109 from runcom/11625-fix-windows-test-build-ca…
dd4389f 
…ncellation

Fix TestBuildCancelationKillsSleep to not fail on Windows
@LK4D4 LK4D4 merged commit dd4389f into moby:master Apr 9, 2015
@runcom runcom mentioned this pull request Apr 9, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

platform/windows status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix TestBuildCancelationKillsSleep so it passes on windows

7 participants

@runcom @LK4D4 @pwaller @jessfraz @duglin @icecrime @GordonTheTurtle