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

Collector: Sequence of shutdown calls resulting in crash due to panic and leads to span loss #2379

Closed
Sreevani871 opened this issue Aug 10, 2020 · 3 comments · Fixed by #2381
Labels

Comments

@Sreevani871
Copy link
Contributor

Describe the bug
Present shutdown sequence calls of collector service is as follows
servers -> writers -> collector_queue_processors(with drain)

First closing storage writers and then draining the collector queue. Which resulting in collector accepting spans until the writers close operation done.
While draining collector queue on issue of collector close operation, collector is trying to write spans to storage since the writer is closed first it resulting in panic and leads to span loss.
send on closed channel

To Reproduce
Steps to reproduce the behavior:

  1. Continuously generate a high volume of traffic to collector service
  2. Stop the collector service process by CTRL + C or soft kill the process.
  3. We can see a panic with error message Send on closed channel and process exit in collector logs

Expected behavior
Ideal shutdown sequence order should be as follows
servers -> queue processors (with drain) -> writers

@ghost ghost added the needs-triage label Aug 10, 2020
@Sreevani871
Copy link
Contributor Author

Present Code
https://github.com/jaegertracing/jaeger/blob/master/cmd/collector/main.go#L97

svc.RunAndThen(func() {
				if closer, ok := spanWriter.(io.Closer); ok {
					err := closer.Close()
					if err != nil {
						logger.Error("failed to close span writer", zap.Error(err))
					}
				}

				if err := c.Close(); err != nil {
					logger.Error("failed to cleanly close the collector", zap.Error(err))
				}
			})

Fix:

svc.RunAndThen(func() {
				if err := c.Close(); err != nil {
					logger.Error("failed to cleanly close the collector", zap.Error(err))
				}
				if closer, ok := spanWriter.(io.Closer); ok {
					err := closer.Close()
					if err != nil {
						logger.Error("failed to close span writer", zap.Error(err))
					}
				}
			})

@jpkrohling
Copy link
Contributor

Would you mind opening a PR with your suggested change?

@Sreevani871
Copy link
Contributor Author

Would you mind opening a PR with your suggested change?

Raised PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants