Inconsistent behavior between `config:get*` accessors during cache failures
- Dominant language
- Erlang
- Stars
- 7k
- Forks
- 1.1k
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 9
Description
[NOTE]: # ( ^^ Provide a general summary of the issue in the title above. ^^ )
## Description
There are two subsets of `config:get*` accessors: 1) baseline `get` in functions `config:get/{1,2,3}`; 2) three wrapper functions around the baseline `get` for type coercion: `get_integer/3`, `get_boolean/3`, and `get_float/3`.
The bug here is that the baseline `get` accessor fails with an ets exception in the event the ets table does not exist, crashing the caller and _not_ providing any config value, whereas using the secondary category of type coercion wrappers _catches_ the exception out of the baseline `get` call and then returns the default value, rather than allowing the exception to bubble up.
The implication of this is that live config lookups can suddenly revert to default values in the event the proper set of values stored in the ets table becomes unavailable. This is obviously not good.
In practice, `config` is pretty reliable at this point, and I only stumbled upon it while debugging eunit tests where we're constantly spinning up and tearing down subsets of CouchDB applications, resulting in numerous instances of config lookup crashes during teardown when there were still live requests but the ets table did not exist. So I don't think this is commonly occurring today, but the type coercion functions landed a decade ago [1], and I've personally run `exit(whereis(config))` on a number of occasions, so I know this happened a few times at least.
This issue is further complicated by the fact that the `try ... catch error:badarg ... end` logic in the typed accessors is at least triple overloaded. The core pattern for all three functions is the following, with `s/integer/float/g` and `s/integer/boolean/g` for `get_integer/3` and `get_boolean/3`, respectively:
```erlang
get_integer(Section, Key, Default) when is_integer(Default) ->
try
to_integer(get(Section, Key, Default))
catch
error:badarg ->
Default
end.
```
The three scenarios I'm aware of that can trigger the catch statement resulting in the Default value flowing through are:
1) it's not the appropriate data type, so conversion functions like `list_to_integer` throw `{error, badarg}`.
2) baseline `get/{1,2,3}` throws `{error, badarg}` when the ets table doesn't exist
3) `get/3` throws `{error, badarg}` itself when the ets:lookup succeeds with zero results and default is a bad data type [2]
## Steps to Reproduce
This can be done independently of CouchDB with the raw `config.beam` file:
```erlang
chewbranca@Russells-MacBook-Pro-2 db % erl -pa src/config/ebin
Erlang/OTP 25 [erts-13.2.2.3] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1]
Eshell V13.2.2.3 (abort with ^G)
1> config:get_boolean("foo", "bar", false).
false
2> config:get("foo", "bar", "true").
** exception error: bad argument
in function ets:lookup/2
called as ets:lookup(config,{"foo","bar"})
*** argument 1: the table identifier does not refer to an existing ETS table
in call from config:get/3 (src/config.erl, line 161)
3>
```
## Expected Behaviour
I don't think there's ever a scenario where we want the user provided settings in default.ini and local.ini to be ignored because the cache disappeared resulting in reverting to unexpected default values. I think we should always crash when the ets table doesn't exist for normal database operations.
I do however think we should wire up the eunit test suite such that we can toggle off use of the ini provide config values and force use of all default values in the code base. We could achieve this by way of something like meck'ing out `config:get/3` as `fun(_Key, _Section, Default) -> Default end.`
On a positive note, it's fairly easy to induce that behavior by way of moving the call to `get/3` in each of the three typed accessors outside of the try-catch block such that we allow lookup exceptions but catch data type exceptions.
The following diff:
```diff
diff --git a/src/config/src/config.erl b/src/config/src/config.erl
index 3ece5326b..30c666bb2 100644
--- a/src/config/src/config.erl
+++ b/src/config/src/config.erl
@@ -68,8 +68,9 @@ all() ->
lists:sort(gen_server:call(?MODULE, all, infinity)).
get_integer(Section, Key, Default) when is_integer(Default) ->
+ Val = get(Section, Key, Default),
try
- to_integer(get(Section, Key, Default))
+ to_integer(Val)
catch
error:badarg ->
Default
@@ -91,8 +92,9 @@ to_integer(Bin) when is_binary(Bin) ->
list_to_integer(binary_to_list(Bin)).
get_float(Section, Key, Default) when is_float(Default) ->
+ Val = get(Section, Key, Default),
try
- to_float(get(Section, Key, Default))
+ to_float(Val)
catch
error:badarg ->
Default
@@ -116,8 +118,9 @@ to_float(Bin) when is_binary(Bin) ->
list_to_float(binary_to_list(Bin)).
get_boolean(Section, Key, Default) when is_boolean(Default) ->
+ Val = get(Section, Key, Default),
try
- to_boolean(get(Section, Key, Default))
+ to_boolean(Val)
catch
error:badarg ->
Default
```
gives the desired behavior:
```erlang
chewbranca@Russells-MBP-2 couchdb % erl -pa src/config/ebin
Erlang/OTP 25 [erts-13.2.2.3] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1]
Eshell V13.2.2.3 (abort with ^G)
1> config:get_boolean("foo", "bar", false).
** exception error: bad argument
in function ets:lookup/2
called as ets:lookup(config,{"foo","bar"})
*** argument 1: the table identifier does not refer to an existing ETS table
in call from config:get/3 (src/config.erl, line 163)
in call from config:get_boolean/3 (src/config.erl, line 121)
2> config:get("foo", "bar", "true").
** exception error: bad argument
in function ets:lookup/2
called as ets:lookup(config,{"foo","bar"})
*** argument 1: the table identifier does not refer to an existing ETS table
in call from config:get/3 (src/config.erl, line 163)
3> %
```
If folks think that's a reasonable approach I can move that over to a PR.
## Additional Context
[1] https://github.com/apache/couchdb-config/commit/6859d11114af0911b91d6e97ac2d87d00c2a3bc3
[2] https://github.com/apache/couchdb-config/blob/master/src/config.erl#L167
Contributor guide
Research direction
Start with src/config/src/config.erl and reproduce the missing-ETS-table behavior using the Erlang shell commands in the issue. Review the eunit configuration tests and verify that ETS lookup failures propagate while type-conversion failures still return the supplied default; done should also cover forcing default values in tests if that approach is retained.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- erlang
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100