Feuerlabs / exometer_core

Core components of exometer
Mozilla Public License 2.0
193 stars 121 forks source link

Exometer cached value potentially not deleted when expired #105

Open lucafavatella opened 6 years ago

lucafavatella commented 6 years ago

When a process calls exometer_cache:write/{3,4}, it:

Depending on the scheduling of the process calling exometer_cache:write and the exometer_cache process (I am excluding process death for simplicity), the cache ETS entry may or may not have the timer ref. If the cache ETS entry has no timer ref, the cache ETS entry is not deleted when the timer fires.

A following exometer_cache:write may update the cache ETS entry, though it looks to me this is not the intended behaviour hence the report.


Sample minimal example (notice the ets:select_delete calls):

Erlang/OTP 20 [erts-9.2] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:10] [hipe] [kernel-poll:false]

Eshell V9.2  (abort with ^G)
1> ets:new(?TABLE, [set, public, named_table, {keypos, 2}]).
* 1: syntax error before: '?'
1> ets:new(exometer_cache, [set, public, named_table, {keypos, 2}]).
exometer_cache
2> ets:insert(exometer_cache, {cache, name, value, undefined, time, ttl}).
true
3> ets:lookup(exometer_cache, name).
[{cache,name,value,undefined,time,ttl}]
4> ets:update_element(exometer_cache, name, {4, tref}).
true
5> ets:lookup(exometer_cache, name).
[{cache,name,value,tref,time,ttl}]
6> ets:update_element(exometer_cache, name, {4, tref2}).
true
7> ets:lookup(exometer_cache, name).
[{cache,name,value,tref2,time,ttl}]
8> ets:insert(exometer_cache, {cache, name, value, undefined, time, ttl}).
true
9> ets:lookup(exometer_cache, name).
[{cache,name,value,undefined,time,ttl}]
10> ets:select_delete(exometer_cache, [{{cache, name, '_', tref2, '_', '_'}, [], [true]}]).
0
11> ets:lookup(exometer_cache, name).
[{cache,name,value,undefined,time,ttl}]
12> ets:update_element(exometer_cache, name, {4, tref3}).
true
13> ets:select_delete(exometer_cache, [{{cache, name, '_', tref3, '_', '_'}, [], [true]}]).
1
14> ets:lookup(exometer_cache, name).
[]
lucafavatella commented 6 years ago

A following exometer_cache:write may update the cache ETS entry, though it looks to me this is not the intended behaviour hence the report.

Actually I see that the piece of code that I understand it is supposed to call exometer_cache:write on a metric-datapoint meant to be cached is conditionally called based on the metric-datapoint having a cached value, so this seems an issue.