Try to make tag.Map threadsafe. #89
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
krnowak
requested review from
bogdandrutu,
jmacd,
pjanotti,
rghetia,
sjkaris and
tedsuo
as code owners
krnowak
force-pushed
the
krnowak/locked-tag-map
branch
from
b872e0d to
3ebf56d
Compare
|
Updated to make |
krnowak
commented
Aug 13, 2019
| @@ -82,68 +109,24 @@ func (m tagMap) Foreach(f func(kv core.KeyValue) bool) { | |||
| } | |||
| } | |||
|
|
|||
| func (m tagMap) apply(mutator Mutator) { | |||
| if m == nil { | |||
There was a problem hiding this comment.
This never happens, so I dropped the check.
jmacd
approved these changes
Aug 16, 2019
rghetia
approved these changes
Aug 23, 2019
|
@krnowak can you please resolve the conflict? |
This is to make tag.Map an immutable type, so it is safe to use concurrently. The safety is not yet fully achieved because of the functions returning contents of the map (Value and Foreach). The functions give callers an access to core.Value objects, which contain a byte slice, which has pointer like semantics. So to avoid accidental changes, we will need to copy the value if it is of BYTES type. Fixes open-telemetry#59
krnowak
force-pushed
the
krnowak/locked-tag-map
branch
from
3ebf56d to
fc8c10e
Compare
|
Updated. And filed #105. |
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.
This adds a lockedMap type which implements tag.Map interface. The
implementation contains tagMap instance and a mutex, so all the
operations are forwarded to the tagMap under the locked mutex.
An additional care was needed for the functions returning contents of
the map, because core.Value contains a byte slice, which has pointer
like semantics. So to avoid accidental changes, we copy the value if
it is of BYTES type. This likely should be handled by the core.Value
itself, e.g. through some Copy function.
The downside of locking here is that the users of Foreach function
need to be careful to not call into the same map, otherwise deadlock
will happen.
While writing this code I think I have got an understanding of the
issue at hand - the implementation of the tag.Map should basically be
immutable (so the modification of the map actually produces a new map,
instead of doing in-place updates, thus making the map threadsafe),
but that is not easy (or even possible) to enforce. So maybe it's
indeed better to avoid providing the ability of having a different,
vendor-specific implementation of tag.Map and have a good-by-default
implemetation as a part of API.
Still, to preserve the immutability of the map, the core.Value structs
need to be deep-copied.
Fixes #59