You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "nickva (via GitHub)" <gi...@apache.org> on 2025/01/31 16:59:53 UTC
[PR] Kill all couchjs processes right after updating the engine setting [couchdb]
nickva opened a new pull request, #5419:
URL: https://github.com/apache/couchdb/pull/5419
Previously, we set the default JS engine dynamically, so that newly spawned processes would use the new setting. However, we didn't actively try to kill the old JS processes. Since a quiet cluster, without much concurrent indexing activity, could hang on to the same set of spawned processes for a while, it may take an indeterminate amount of time until the couchjs processes actually switch to the new engine.
To fix that let's force a `reload()` as well, which will immediately kill all the free (available) processes and then kill the acquired processes when the user is done with them and releases them back to the process manager.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Kill all couchjs processes right after updating the engine setting [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5419:
URL: https://github.com/apache/couchdb/pull/5419#discussion_r1937631585
##########
src/couch_quickjs/test/couch_quickjs_tests.erl:
##########
@@ -78,14 +78,43 @@ t_couch_jsengine_config_triggers_proc_server_reload(_) ->
<<"spidermonkey">> -> "quickjs"
end,
+ % Get and return one proc to the pool, and get and keep another
+ % proc during the switch. Both should be eventually be killed.
+
+ {ok, ProcKeep, _} = couch_proc_manager:get_proc(<<"javascript">>),
+ PidKeep = element(2, ProcKeep),
+ RefKeep = monitor(process, PidKeep),
+ ?assert(is_pid(PidKeep)),
+
+ {ok, ProcFree, _} = couch_proc_manager:get_proc(<<"javascript">>),
+ PidFree = element(2, ProcFree),
+ RefFree = monitor(process, PidFree),
+ % Return it back so there is one free process in the pool
+ couch_proc_manager:ret_proc(ProcFree),
+
config:set("couchdb", "js_engine", Toggle, false),
% couch_server:get_js_engine/0 should be visible immediately
?assertEqual(list_to_binary(Toggle), couch_server:get_js_engine()),
wait_until_proc_manager_updates(OldVal),
?assertNotEqual(OldVal, get_proc_manager_default_js()),
- NewVal = get_proc_manager_default_js(),
+ % Wait for the returned process to die
+ receive
+ {'DOWN', RefFree, _, _, _} -> ok
+ end,
+
+ % The one we kept should be alive, we don't want to discrupt
+ % checked-out processes while they are doing work
+ is_process_alive(PidKeep),
Review Comment:
fixed, thank you, @jiahuili430 !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Kill all couchjs processes right after updating the engine setting [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva merged PR #5419:
URL: https://github.com/apache/couchdb/pull/5419
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Kill all couchjs processes right after updating the engine setting [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5419:
URL: https://github.com/apache/couchdb/pull/5419#discussion_r1937628885
##########
src/couch_quickjs/test/couch_quickjs_tests.erl:
##########
@@ -78,14 +78,43 @@ t_couch_jsengine_config_triggers_proc_server_reload(_) ->
<<"spidermonkey">> -> "quickjs"
end,
+ % Get and return one proc to the pool, and get and keep another
+ % proc during the switch. Both should be eventually be killed.
+
+ {ok, ProcKeep, _} = couch_proc_manager:get_proc(<<"javascript">>),
+ PidKeep = element(2, ProcKeep),
+ RefKeep = monitor(process, PidKeep),
+ ?assert(is_pid(PidKeep)),
+
+ {ok, ProcFree, _} = couch_proc_manager:get_proc(<<"javascript">>),
+ PidFree = element(2, ProcFree),
+ RefFree = monitor(process, PidFree),
+ % Return it back so there is one free process in the pool
+ couch_proc_manager:ret_proc(ProcFree),
+
config:set("couchdb", "js_engine", Toggle, false),
% couch_server:get_js_engine/0 should be visible immediately
?assertEqual(list_to_binary(Toggle), couch_server:get_js_engine()),
wait_until_proc_manager_updates(OldVal),
?assertNotEqual(OldVal, get_proc_manager_default_js()),
- NewVal = get_proc_manager_default_js(),
+ % Wait for the returned process to die
+ receive
+ {'DOWN', RefFree, _, _, _} -> ok
+ end,
+
+ % The one we kept should be alive, we don't want to discrupt
+ % checked-out processes while they are doing work
+ is_process_alive(PidKeep),
Review Comment:
oh yes, good call, that as meant to be an assert!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Kill all couchjs processes right after updating the engine setting [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #5419:
URL: https://github.com/apache/couchdb/pull/5419#discussion_r1937627001
##########
src/couch_quickjs/test/couch_quickjs_tests.erl:
##########
@@ -78,14 +78,43 @@ t_couch_jsengine_config_triggers_proc_server_reload(_) ->
<<"spidermonkey">> -> "quickjs"
end,
+ % Get and return one proc to the pool, and get and keep another
+ % proc during the switch. Both should be eventually be killed.
+
+ {ok, ProcKeep, _} = couch_proc_manager:get_proc(<<"javascript">>),
+ PidKeep = element(2, ProcKeep),
+ RefKeep = monitor(process, PidKeep),
+ ?assert(is_pid(PidKeep)),
+
+ {ok, ProcFree, _} = couch_proc_manager:get_proc(<<"javascript">>),
+ PidFree = element(2, ProcFree),
+ RefFree = monitor(process, PidFree),
+ % Return it back so there is one free process in the pool
+ couch_proc_manager:ret_proc(ProcFree),
+
config:set("couchdb", "js_engine", Toggle, false),
% couch_server:get_js_engine/0 should be visible immediately
?assertEqual(list_to_binary(Toggle), couch_server:get_js_engine()),
wait_until_proc_manager_updates(OldVal),
?assertNotEqual(OldVal, get_proc_manager_default_js()),
- NewVal = get_proc_manager_default_js(),
+ % Wait for the returned process to die
+ receive
+ {'DOWN', RefFree, _, _, _} -> ok
+ end,
+
+ % The one we kept should be alive, we don't want to discrupt
+ % checked-out processes while they are doing work
+ is_process_alive(PidKeep),
Review Comment:
Do we need `?assert` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org