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

Fix hanging during shutdown and logstash output #1

Closed
wants to merge 2 commits into from

Conversation

mprecise
Copy link

I don't know go, so this is most likely not a great fix, but it fixes the issues I was having.

My test system is running Ubuntu 14.04 LTS, if that matters.

Symptom 1.

execbeat_console.yml

execbeat:
  execs:
    -
      cron: "@every 5s"
      command: echo
      args: "hello"
      document_type: execbeat 

output:
   console:
/usr/bin/execbeat -e -c execbeat_console.yml -d "publish"

Pressing ctrl+c gives:

^Cpanic: close of nil channel

goroutine 24 [running]:
github.com/christiangalsterer/execbeat/beat.(*Execbeat).Stop(0xc8200fe2d0)
    /home/ubuntu/go/src/github.com/christiangalsterer/execbeat/beat/execbeat.go:61 +0x24

Symptom 2.

Sometimes pressing ctrl+c does not close the process. It must be killed with sudo kill -9 [pid]

Symptom 3.

logstash.conf

input {
    beats {
        port => 5044
    }
}

output {
    stdout {
        codec => rubydebug
    }
}
/opt/logstash/bin/logstash -f logstash.conf

execbeat_logstash.yml

execbeat:
  execs:
    -
      cron: "@every 5s"
      command: echo
      args: "hello"
      document_type: execbeat 

output:
  logstash:
    hosts: ["localhost:5044"]
/usr/bin/execbeat -e -c execbeat_logstash.yml -d "publish"

execbeat hangs after the third publish. In this case it must always be killed with kill -9.

Explanation

I believe the infinite busy loop for { } in execbeat.go was only used to keep the main thread from exiting while Executor does its thing. I also believe it's the main culprit of the problems.

I examined topbeat and saw that it used a Ticker to schedule events and as a side effect, to keep the main thread running while waiting until it's time to trigger the next event.

For execbeat, we don't need to use Ticker to schedule events since that's handled by Executor, so with my fix we're merely using Ticker's side effect of keeping the main thread running. This seems like a bit of a hack to me, but with no experience in go, I'm not sure what the recommended strategy is.

With my code changes in place, my manual testing shows that ctrl+c cleanly shuts down the program, and using logstash as an output no longer hangs after just a few publishes.

@codecov-io
Copy link

Current coverage is 9.80%

Merging #1 into master will decrease coverage by -0.61% as of da52aac

@@            master    #1   diff @@
====================================
  Files            4     4       
  Stmts           96   102     +6
  Branches         1     1       
  Methods          0     0       
====================================
  Hit             10    10       
  Partial          0     0       
- Missed          86    92     +6

Review entire Coverage Diff as of da52aac

Powered by Codecov. Updated on successful CI builds.

@christiangalsterer
Copy link
Owner

@mprecise : Thanks for the PR. I will look into it in the next days.

@christiangalsterer
Copy link
Owner

I can confirm symptom 1 and 2, but 3 I can't reproduce so far.

@christiangalsterer
Copy link
Owner

I haven't merged your PR but used some of the code, e.g. the ticker was not necessary. Symptoms 1 + 2 I was not able to reproduce after the fix. Would be great if you can test the new release 1.0.1.
Please note that I was not able to reproduce symptom 3.

@mprecise
Copy link
Author

I only very briefly looked at this today and didn't notice the symptoms. I will spend a bit more time with it tomorrow running on a machine that is more like my production systems.

@mprecise mprecise mentioned this pull request Feb 17, 2016
@mprecise
Copy link
Author

No longer needed after 1.0.1. Thanks!

@mprecise mprecise closed this Feb 17, 2016
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