Fix panic when a signal is delivered before the server is instantiated #580
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
January 30, 2018 15:36
…server before assigning it (ie. read before write) fixes elastic#514
graphaelli
requested changes
Jan 30, 2018
CHANGELOG.asciidoc
Outdated
| @@ -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] | |||
There was a problem hiding this comment.
Bit of a nitpick but we currently catch INT and TERM, maybe just signal instead?
beater/beater.go
Outdated
| @@ -7,14 +7,18 @@ import ( | |||
| "net/http" | |||
| "regexp" | |||
|
|
|||
| "sync" | |||
There was a problem hiding this comment.
not sure how this is getting through the make check - can you group this with the other stdlib imports?
There was a problem hiding this comment.
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.
beater/beater.go
Outdated
| server *http.Server | ||
| config *Config | ||
| server *http.Server | ||
| mutex sync.RWMutex |
There was a problem hiding this comment.
mutex is guarding both server and stopped - can you move it above and add a comment to that effect like here?
| @@ -36,7 +40,8 @@ func New(b *beat.Beat, ucfg *common.Config) (beat.Beater, error) { | |||
| } | |||
|
|
|||
| bt := &beater{ | |||
| config: beaterConfig, | |||
| config: beaterConfig, | |||
| stopped: false, | |||
There was a problem hiding this comment.
I like this being explicit 👍
|
thanks Gil, comments addressed here 0731cbe |
marclop
reviewed
Jan 31, 2018
beater/beater.go
Outdated
| config *Config | ||
| server *http.Server | ||
| config *Config | ||
| mutex sync.RWMutex // guards server and stopped |
There was a problem hiding this comment.
no need for a RWMutex if RLock and RUnlock isn't used.
graphaelli
changed the title
graphaelli
approved these changes
Jan 31, 2018
jalvz
added a commit
to jalvz/apm-server
that referenced
this pull request
Feb 1, 2018
elastic#580) Introduce a flag and a mutex in the beater so that we can't stop the server before assigning it (ie. read before write) fixes elastic#514
jalvz
added a commit
to jalvz/apm-server
that referenced
this pull request
Feb 1, 2018
elastic#580) Introduce a flag and a mutex in the beater so that we can't stop the server before assigning it (ie. read before write) fixes elastic#514
jalvz
added a commit
to jalvz/apm-server
that referenced
this pull request
Feb 1, 2018
elastic#580) Introduce a flag and a mutex in the beater so that we can't stop the server before assigning it (ie. read before write) fixes elastic#514
jalvz
added a commit
to jalvz/apm-server
that referenced
this pull request
Feb 1, 2018
elastic#580) Introduce a flag and a mutex in the beater so that we can't stop the server before assigning it (ie. read before write) fixes elastic#514
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a flag and a mutex in the beater so that we can't stop the server before assigning it (ie. read before write)
fixes #514