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

Replaces agent infinite block with agent.Stop() #937

Closed
wants to merge 1 commit into from

Conversation

danehans
Copy link

Which problem is this PR solving?

  • The select {} statement to place agent in an infinite loop is uncommon and causes confusion for new contributors.

Short description of the changes

  • The agent.Stop() method exists, so let's use it. Calling defer with the agent.Stop() method will stop agent only when the agent.Start go routine stops/fails and is easier for new contributors to understand.

Signed-off-by: Daneyon Hansen [email protected]

@sergeyklay
Copy link

@black-adder Aha! https://travis-ci.org/jaegertracing/jaeger/jobs/405032533#L3365
The same crossdock error I faced yesterday in the #933

@@ -63,7 +63,8 @@ func main() {
if err := agent.Run(); err != nil {
return errors.Wrap(err, "Failed to run the agent")
}
select {}
defer agent.Stop()
return nil
Copy link
Member

Choose a reason for hiding this comment

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

I think this will exit the app immediately, won't it?

@yurishkuro
Copy link
Member

The select {} statement to place agent in an infinite loop is uncommon and causes confusion for new contributors.

It's not that uncommon. But in the other executables we trapped the interrupts:

jaeger/cmd/query/main.go

Lines 143 to 147 in 7919cd9

select {
case <-serverChannel:
logger.Info("Jaeger Query is finishing")
}
return nil

@danehans
Copy link
Author

@yurishkuro OK. I tested locally and the agent does exit immediately. Although it would be nice to defer the agent.Stop(), it's not required. I will close out this PR. Thank you.

@danehans danehans closed this Jul 18, 2018
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.

3 participants