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

single door out for main job loop #59

Merged
merged 1 commit into from
Oct 22, 2020
Merged

Conversation

g-pavlov
Copy link
Contributor

@g-pavlov g-pavlov commented Oct 22, 2020

What this PR does / why we need it:
The current job wait loop can be exited either by a signal to quit and after all workers have quit, or when the combined workers error channel is closed, assuming all workers have quit by then.

However closing a channel yields signal itself and it may happen that the case waiting on workers error channel may receive the last signal and exit the main loop before the case for quit has finished waiting for workers, which will yield close of the quit channel with any not completely finished workers panicking while trying to send message they are done.

panic: send on closed channel

goroutine 91 [running]:
github.com/gardener/docforge/pkg/jobs.(*Job).startWorkers.func1.1(0xc0002961e0, 0xc000096900, 0xc0002860a0, 0x3)
	/tmp/build/a94a8fe5/git-gardener_docforge-master_master/pkg/jobs/jobs.go:223 +0x4a
github.com/gardener/docforge/pkg/jobs.(*Job).startWorkers.func1(0xc00028a060, 0xc0002860a0, 0xc000284074, 0x7bc0e0, 0xc000280720, 0x3, 0x7bc2a0, 0xc00028a040, 0xc0002961e0)
	/tmp/build/a94a8fe5/git-gardener_docforge-master_master/pkg/jobs/jobs.go:240 +0x386
created by github.com/gardener/docforge/pkg/jobs.(*Job).startWorkers
	/tmp/build/a94a8fe5/git-gardener_docforge-master_master/pkg/jobs/jobs.go:219 +0x127

The change in the PR is to ensure main loop exit only in the quit case, ensuring all workers quit before exiting main loop under any circumstances.

Release note:

Fixes an occasional `panic: send on closed channel` while finishing a build

@g-pavlov g-pavlov added the component/documentation Gardener Documentation label Oct 22, 2020
@g-pavlov g-pavlov self-assigned this Oct 22, 2020
@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Oct 22, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 22, 2020
@g-pavlov g-pavlov changed the title wait all workers to exit also on error single door out for main job loop Oct 22, 2020
@gardener-robot gardener-robot removed the needs/review Needs review label Oct 22, 2020
@g-pavlov g-pavlov merged commit 5d4dbcb into master Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/documentation Gardener Documentation needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants