Do not crash the server on invalid js_engine setting#5329
Conversation
In the previous PR [1] Jessica had discovered that an invalid engine setting crashes the server. Initially I had thought we would be strict about it and just let it crash, but that seems a bit harsh for this. Especially if there is a large cluster and configuration is pushed to all nodes. It would crash the whole cluster. Instead, since we have a default, stick with that. Especially now users can validate if a non-default js_engine is active from the welcome endpoint. [1] #5327
| % If user misconfigured it by accident, don't crash, use defaults | ||
| case lists:member(Engine, ?AVAILABLE_JS_ENGINES) of | ||
| true -> list_to_binary(Engine); | ||
| false -> list_to_binary(?COUCHDB_JS_ENGINE) |
There was a problem hiding this comment.
Shall we print some error message to let the user know something went wrong and config:set/3 to the default js_engine ?
(node1@127.0.0.1)1> couch_server:get_js_engine().
<<"quickjs">>
(node1@127.0.0.1)2> config:set("couchdb", "js_engine", "spidermonkey").
ok
(node1@127.0.0.1)3> couch_server:get_js_engine().
<<"spidermonkey">>
(node1@127.0.0.1)4> config:set("couchdb", "js_engine", "wrong_engine").
ok
(node1@127.0.0.1)5> couch_server:get_js_engine().
<<"wrong_engine">>There was a problem hiding this comment.
Hmm
(node1@127.0.0.1)4> config:set("couchdb", "js_engine", "wrong_engine").
should have resulted in the default being returned
(node1@127.0.0.1)4> config:set("couchdb", "js_engine", "wrong_engine").
ok
(node1@127.0.0.1)5> couch_server:get_js_engine().
<<"spidermonkey">>
Did you re-run make at the top level, wonder if the module didn't compile properly...
There is no good place to log it from since it all takes place in config:set/3. Doing it in couch_server:get_js_engine() might not be a good idea as that's called from the welcome endpoint so we wouldn't want to log errors every call there since it's completely un-authenticated. With the welcome endpoint returning the non-default the users should be able to figure out if the setting took effect or not
There was a problem hiding this comment.
It turns out my icu4c was outdated and make could not compile the code correctly.
$ brew reinstall icu4c@75
$ ./dev/remsh
(node1@127.0.0.1)1> couch_server:get_js_engine().
<<"quickjs">>
(node1@127.0.0.1)2> config:set("couchdb", "js_engine", "spidermonkey").
ok
(node1@127.0.0.1)3> couch_server:get_js_engine().
<<"spidermonkey">>
(node1@127.0.0.1)4> config:set("couchdb", "js_engine", "wrong_engine").
ok
(node1@127.0.0.1)5> couch_server:get_js_engine().
<<"quickjs">>Works as expected, +1!
2 participants
In the previous PR [1] Jessica had discovered that an invalid engine setting crashes the server. Initially I had thought we would be strict about it and just let it crash, but that seems a bit harsh for this. Especially if there is a large cluster and configuration is pushed to all nodes. It would crash the whole cluster.
Instead, since we have a default, stick with that. Especially now users can validate if a non-default js_engine is active from the welcome endpoint.
[1] #5327