Skip to content

Do not crash the server on invalid js_engine setting #5329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nickva merged 1 commit into from
Oct 31, 2024
Merged

Do not crash the server on invalid js_engine setting #5329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/couch/src/couch_server.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
-define(MAX_DBS_OPEN, 500).
-define(RELISTEN_DELAY, 5000).
-define(DEFAULT_ENGINE, "couch").
-define(AVAILABLE_JS_ENGINES, ["spidermonkey", "quickjs"]).

-record(server, {
root_dir = [],
Expand Down Expand Up @@ -92,7 +93,12 @@ get_stats() ->
[{start_time, ?l2b(Time)}, {dbs_open, Open}].

get_js_engine() ->
list_to_binary(config:get("couchdb", "js_engine", ?COUCHDB_JS_ENGINE)).
Engine = config:get("couchdb", "js_engine", ?COUCHDB_JS_ENGINE),
% 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)
Copy link
Copy Markdown
Contributor

@jiahuili430 jiahuili430 Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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">>

Copy link
Copy Markdown
Contributor Author

@nickva nickva Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@jiahuili430 jiahuili430 Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

end.

get_spidermonkey_version() -> list_to_binary(?COUCHDB_SPIDERMONKEY_VERSION).

Expand Down
23 changes: 22 additions & 1 deletion src/couch_quickjs/test/couch_quickjs_tests.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ quickjs_test_() ->
?TDEF_FE(t_get_coffee_cmd),
?TDEF_FE(t_can_configure_memory_limit),
?TDEF_FE(t_bad_memory_limit),
?TDEF_FE(t_couch_jsengine_config_triggers_proc_server_reload)
?TDEF_FE(t_couch_jsengine_config_triggers_proc_server_reload),
?TDEF_FE(t_invalid_jsengine_config)
]
}.

Expand Down Expand Up @@ -93,6 +94,26 @@ t_couch_jsengine_config_triggers_proc_server_reload(_) ->
?assertEqual(OldVal, get_proc_manager_default_js())
end.

t_invalid_jsengine_config(_) ->
config:delete("couchdb", "js_engine", false),
Default = couch_server:get_js_engine(),
ProcManagerPid = whereis(couch_proc_manager),
?assert(is_process_alive(ProcManagerPid)),
% Try invalid settings
config:set("couchdb", "js_engine", "non_existent_test_engine", false),
% Wait to make sure couch_proc_manager hasn't crashed
timer:sleep(100),
?assertEqual(ProcManagerPid, whereis(couch_proc_manager)),
?assertEqual(Default, couch_server:get_js_engine()),
% Use valid settings
config:set("couchdb", "js_engine", "spidermonkey", false),
?assertEqual(<<"spidermonkey">>, couch_server:get_js_engine()),
config:set("couchdb", "js_engine", "quickjs", false),
?assertEqual(<<"quickjs">>, couch_server:get_js_engine()),
% Return back to the defaults by deleting the config
config:delete("couchdb", "js_engine", false),
?assertEqual(Default, couch_server:get_js_engine()).

os_cmd(Cmd) ->
Opts = [stream, {line, 4096}, binary, exit_status, hide],
Port = open_port({spawn, Cmd}, Opts),
Expand Down