worker/terminationworker: fix test hang #5528
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes LP 1588135 Fix a race on the ownership of the abort signal. The TestSignal test attempts to see if sending the SIGABRT to the test binary itself will cause the worker to unblock itself and exit as exited. However, as the original author discovered there is a race between sending the signal and the worker spawned from NewWorker registering a handler to catch SIGABRT. If the hander was not registered in time the SIGABRT would cause the test process to exit. To fix this problem they set up a SIGABRT handler in SetUpTest to ensure that SIGABRT was always handled. However this meant that if NewWorker's handler was not registered in time to catch the signal, it would be delivered to the channel registed by SetUpTest, causing the test binary to hang. The fix is to remove the SetUpTest workaround and ensure the handler is always registered before spawning the loop goroutine so the caller is assured that once NewWorker has returned, the handler is in place and it is safe to send the signal. This PR also removes unnecessary use of testing.BaseSuite.
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes LP 1588135
Fix a race on the ownership of the abort signal.
The
TestSignaltest attempts to see if sendingSIGABRTto the testbinary itself will cause the worker to unblock itself and exit as
exited. However, as the original author discovered there is a race
between sending the signal and the worker spawned from
NewWorkerregistering a handler to catch
SIGABRT. If the handler was not registeredin time the
SIGABRTwould cause the test process to exit.To fix this problem they set up a
SIGABRThandler inSetUpTestto ensurethat
SIGABRTwas always handled. However this meant that ifNewWorker'shandler was not registered in time to catch the signal, it would be
delivered to the channel registered by
SetUpTest, causing the test binaryto hang.
The fix is to remove the
SetUpTestworkaround and ensure the handler isalways registered before spawning the loop goroutine so the caller is
assured that once
NewWorkerhas returned, the handler is in place and itis safe to send the signal.
This PR also removes unnecessary use of
testing.BaseSuite.(Review request: http://reviews.vapour.ws/r/4975/)