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 panic when a signal is delivered before the server is instantiated #580

Merged
merged 4 commits into from
Feb 1, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ https://github.com/elastic/apm-server/compare/71df0d96445df35afe27f38bcf734a0828
==== Bug fixes
- Updated systemd doc url {pull}354[354]
- Updated readme doc urls {pull}356[356]
- Use updated stack trace frame values for calculating error `grouping_keys` {pull}485[485]
- Use updated stack trace frame values for calculating error `grouping_keys` {pull}485[485]
- Fix panic when a SIGINT is delivered before the server is instantiated {pull}580[580]
Copy link
Member

Choose a reason for hiding this comment

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

Bit of a nitpick but we currently catch INT and TERM, maybe just signal instead?


==== Added
- Include build time and revision in version information {pull}396[396]
Expand Down
25 changes: 21 additions & 4 deletions beater/beater.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,18 @@ import (
"net/http"
"regexp"

"sync"
Copy link
Member

Choose a reason for hiding this comment

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

not sure how this is getting through the make check - can you group this with the other stdlib imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always have this problem, this is how it looks after running gofmt and goimports. Eventually got tired of fixing white lines in import blocks manually... it might be because of my IDE, but make check seems to be happy with this indeed.


"github.com/elastic/beats/libbeat/beat"
"github.com/elastic/beats/libbeat/common"
"github.com/elastic/beats/libbeat/logp"
)

type beater struct {
config *Config
server *http.Server
config *Config
server *http.Server
mutex sync.RWMutex
Copy link
Member

Choose a reason for hiding this comment

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

mutex is guarding both server and stopped - can you move it above and add a comment to that effect like here?

stopped bool
}

// Creates beater
Expand All @@ -36,7 +40,8 @@ func New(b *beat.Beat, ucfg *common.Config) (beat.Beater, error) {
}

bt := &beater{
config: beaterConfig,
config: beaterConfig,
stopped: false,
Copy link
Member

Choose a reason for hiding this comment

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

I like this being explicit 👍

}
return bt, nil
}
Expand All @@ -57,7 +62,14 @@ func (bt *beater) Run(b *beat.Beat) error {
}
go notifyListening(bt.config, pub.Send)

bt.mutex.Lock()
if bt.stopped {
defer bt.mutex.Unlock()
return nil
}

bt.server = newServer(bt.config, pub.Send)
bt.mutex.Unlock()

err = run(bt.server, lis, bt.config)
if err == http.ErrServerClosed {
Expand All @@ -70,5 +82,10 @@ func (bt *beater) Run(b *beat.Beat) error {
// Graceful shutdown
func (bt *beater) Stop() {
logp.Info("stopping apm-server...")
stop(bt.server, bt.config.ShutdownTimeout)
bt.mutex.Lock()
if bt.server != nil {
stop(bt.server, bt.config.ShutdownTimeout)
}
bt.stopped = true
bt.mutex.Unlock()
}