Improve thread safety of fingerprint processor #18738
Merged
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
urso
added
bug
review
libbeat
Team:Services
|
Pinging @elastic/integrations-services (Team:Services) |
botelastic
bot
added
needs_team
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
ycombinator
reviewed
May 26, 2020
| // The fields array must be sorted, to guarantee that we always | ||
| // get the same hash for a similar set of configured keys. | ||
| // The call `ToSlice` always returns a sorted slice. | ||
| fields := common.MakeStringSet(config.Fields...).ToSlice() |
There was a problem hiding this comment.
Out of curiosity, is there a difference in calling .ToSlice() here vs. how it was before, a few lines below? Either way the value of p.fields would be sorted, no? I'm just wondering if you are making this change to try and do all the operations on config.Fields in one place (this line) or if there's some other more fundamental reason that could prevent a bug or something else.
There was a problem hiding this comment.
I just moved it here in order to 'cleanly' add the code comment. Operationally it makes no difference. It's not obvious that ToSlice already sorts and not immediately obvious that we always need the keys to be sorted, so we should document this (at first I thought we have a bug thanks to StringSet storing keys in randomized order).
Often I prefer to have fully 'initialized' variables that I don't need to modify or execute other operations on when constructing another object. This gives each code block a self-contained purpose (almost like a function).
ycombinator
approved these changes
May 26, 2020
|
jenkins please run tests. |
|
jenkins run tests |
urso
force-pushed
the
fingerprint-threadsafe
branch
from
944979d to
db11ece
Compare
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Test errorsExpand to view the tests failures
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
added 3 commits
June 2, 2020 17:01
The fingerprint processor used to reuse state between calls, but not being threadsafe means that the processor might get into an invalid state if it is used by multiple go-routines by accident, creating invalid, non-reproducible hash values. This change does not reuse the hash state between runs. Running the benchmarks seems to suggest that we don't create any extra allocations with this change (tested with go1.13): $ go test -bench . -benchmem goos: darwin goarch: amd64 pkg: github.com/elastic/beats/v7/libbeat/processors/fingerprint BenchmarkHashMethods/sha384-4 1000000000 0.123 ns/op 0 B/op 0 allocs/op BenchmarkHashMethods/sha512-4 1000000000 0.125 ns/op 0 B/op 0 allocs/op BenchmarkHashMethods/xxhash-4 1000000000 0.0539 ns/op 0 B/op 0 allocs/op BenchmarkHashMethods/md5-4 1000000000 0.0925 ns/op 0 B/op 0 allocs/op BenchmarkHashMethods/sha1-4 1000000000 0.112 ns/op 0 B/op 0 allocs/op BenchmarkHashMethods/sha256-4 1000000000 0.134 ns/op 0 B/op 0 allocs/op
urso
force-pushed
the
fingerprint-threadsafe
branch
from
db11ece to
aba010a
Compare
|
Test failures are unrelated. |
3 tasks
urso
pushed a commit
to urso/beats
that referenced
this pull request
Jun 3, 2020
(cherry picked from commit 6c4734b)
urso
added
needs_backport
urso
pushed a commit
that referenced
this pull request
Jun 4, 2020
3 tasks
urso
pushed a commit
to urso/beats
that referenced
this pull request
Jul 3, 2020
(cherry picked from commit 6c4734b)
leweafan
pushed a commit
to leweafan/beats
that referenced
this pull request
Apr 28, 2023
…t processor (elastic#19653) (cherry picked from commit a73ba61)
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.
What does this PR do?
Remove the reuse of the hash function between runs in the hash processor.
Running the benchmarks seems to suggest that we don't create any extra
allocations with this change (tested with go1.13):
Why is it important?
The fingerprint processor used to reuse state between calls, but not
being threadsafe means that the processor might get into an invalid
state if it is used by multiple go-routines by accident, creating
invalid, non-reproducible hash values. This change does not reuse the
hash state between runs.
Checklist
- [ ] I have made corresponding changes to the documentation- [ ] I have made corresponding change to the default configuration files- [ ] I have added tests that prove my fix is effective or that my feature worksCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Related issues