Fix a race condition on add_host_metadata

[jsoriano]

Not sure if adding a mutex at this level will be too blocking for events processing, I have also though on other options to reduce contention on reads:

• Using a RWLock, but we'd still need to write-lock first to check expiration and update
• Using a RWLock and a go-routine to do periodic updates, but afaik we wouldn't have a way to stop it if the processor is removed.

Continues with #8223

[adriansr]

Can we use an atomic reference to the map and guarantee that is in immutable?
As in, loadData should allocate a new map and update the atomic reference, and don't update it once the reference is set (i.e. don't do p.data.Put(...)).

I think a mutex is overkill but I may not be understanding the problem

[exekias]

I wonder if MapStrPointer would help with these cases: https://github.com/elastic/beats/blob/master/libbeat/common/mapstr_pointer.go

[jsoriano]

Not sure about using an atomic pointer, we still need to check and update the last update timestamp under the same mutex zone 🤔

[urso]

This introduces another potential race. Better: p.data.Get().Clone()

[exekias]

just for my understanding, does that mean that the write on data could be coming from some processor in the pipeline? this processor itself is only using set, while treating the inserted MapStr as immutable.

[urso]

Right. Processors or even the output can add/modify the event. E.g. users adding more fields to host (this did actually happen):

fields:
  host.whatever: x
fields_under_root: true

If an user does this to one input only, then the shared data must not be updated at all (so to guarantee data consistency of other modules/inputs). Plus: modifying shared data can race with the serialisation in our outputs.

Working with events and shared data + custom processors is full of pitfalls and potential race conditions. We will have to think about some 'event-type' that can share data (such that we do not create this much garbage), but yet, does prevent developers from accidentally overwriting shared data.

As processors can be run in parallel, caching with concurrent updates is another pitfall in processors.

These issues are known to the Core Team. We hope to provide API/libs dealing with these potential races/pitfalls for you, such that input/processor developers don't have to deal with these kind of issues. At times It's hard to get this right, even if you are aware of these issues.

[jsoriano]

Using MapStrPointer for atomic reference of data, not sure about lastUpdate, I guess that at least we can have multiple updates of data if time expired.

[ph]

@jsoriano Thanks for making that changes, I didn't thought that just cloning wasn't enought but yes in that case using MapStrPointer sound like a better idea.

I think we have to come up with a better story with immutability inside the pipeline and we deal with events.

[ph]

With the usage of the pointer and using clone on Get this sound like a better strategy.

[adriansr]

Suggestion

[ph]

I have created the following issue #8662 to discuss a core implementation of caching.