You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/01/15 17:49:34 UTC
[GitHub] [couchdb] nickva opened a new pull request #3329: Do not return broken processes to the query process pool
nickva opened a new pull request #3329:
URL: https://github.com/apache/couchdb/pull/3329
Previously, if an error was thrown in a `with_ddoc_proc/2` callback, the process was still returned to the process pool in the `after` clause. However, in some cases, for example when processing a `_list` response, the process might
end up stuck in a bad state state, such that it could not be re-used anymore. In such a case, a subsequent user of that `couch_js` process would end up throwing an error and crashing.
Fixes #2962
## Checklist
- [x] Code is written and works correctly
- [x] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva merged pull request #3329: Do not return broken processes to the query process pool
Posted by GitBox <gi...@apache.org>.
nickva merged pull request #3329:
URL: https://github.com/apache/couchdb/pull/3329
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp commented on a change in pull request #3329: Do not return broken processes to the query process pool
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #3329:
URL: https://github.com/apache/couchdb/pull/3329#discussion_r558501958
##########
File path: src/couch/test/eunit/couchdb_os_proc_pool.erl
##########
@@ -205,6 +208,46 @@ should_reduce_pool_on_idle_os_procs() ->
end).
+should_not_return_broken_process_to_the_pool() ->
+ ?_test(begin
+ config:set("query_server_config",
+ "os_process_soft_limit", "1", false),
+ ok = confirm_config("os_process_soft_limit", "1"),
+
+ config:set("query_server_config",
+ "os_process_limit", "1", false),
+ ok = confirm_config("os_process_limit", "1"),
+
+ DDoc = #doc{
+ id = <<"ddoc1">>,
+ revs = {1, [<<"abc">>]},
+ body = {[
+ {<<"language">>, <<"javascript">>},
+ {<<"views">>, {[
+ {<<"v1">>, {[
+ {<<"map">>, <<"function(doc) {emit(doc.value,1);}">>}
+ ]}}
+ ]}}
+ ]}
+ },
+ meck:reset(couch_os_process),
+
+ ?assertEqual(0, couch_proc_manager:get_proc_count()),
+ ok = couch_query_servers:with_ddoc_proc(DDoc, fun(_) -> ok end),
+ ?assertEqual(0, meck:num_calls(couch_os_process, stop, 1)),
+ ?assertEqual(1, couch_proc_manager:get_proc_count()),
+
+ ?assertError(bad, couch_query_servers:with_ddoc_proc(DDoc, fun(_) ->
+ error(bad)
+ end)),
+ ?assertEqual(1, meck:num_calls(couch_os_process, stop, 1)),
Review comment:
Seems like we should add an assertion that there are zero os processes here?
##########
File path: src/couch/test/eunit/couchdb_os_proc_pool.erl
##########
@@ -205,6 +208,46 @@ should_reduce_pool_on_idle_os_procs() ->
end).
+should_not_return_broken_process_to_the_pool() ->
+ ?_test(begin
+ config:set("query_server_config",
+ "os_process_soft_limit", "1", false),
+ ok = confirm_config("os_process_soft_limit", "1"),
+
+ config:set("query_server_config",
+ "os_process_limit", "1", false),
+ ok = confirm_config("os_process_limit", "1"),
+
+ DDoc = #doc{
+ id = <<"ddoc1">>,
+ revs = {1, [<<"abc">>]},
+ body = {[
+ {<<"language">>, <<"javascript">>},
+ {<<"views">>, {[
+ {<<"v1">>, {[
+ {<<"map">>, <<"function(doc) {emit(doc.value,1);}">>}
+ ]}}
+ ]}}
+ ]}
+ },
+ meck:reset(couch_os_process),
+
+ ?assertEqual(0, couch_proc_manager:get_proc_count()),
+ ok = couch_query_servers:with_ddoc_proc(DDoc, fun(_) -> ok end),
+ ?assertEqual(0, meck:num_calls(couch_os_process, stop, 1)),
+ ?assertEqual(1, couch_proc_manager:get_proc_count()),
+
+ ?assertError(bad, couch_query_servers:with_ddoc_proc(DDoc, fun(_) ->
+ error(bad)
+ end)),
+ ?assertEqual(1, meck:num_calls(couch_os_process, stop, 1)),
+
+ ok = couch_query_servers:with_ddoc_proc(DDoc, fun(_) -> ok end),
Review comment:
I'm pretty sure the original bug required a second design doc to be sent to the process in this part to trigger the bug, should we do that to be sure? Should be able to just tweak the id or revs of DDoc.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3329: Do not return broken processes to the query process pool
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3329:
URL: https://github.com/apache/couchdb/pull/3329#discussion_r558510996
##########
File path: src/couch/test/eunit/couchdb_os_proc_pool.erl
##########
@@ -205,6 +208,46 @@ should_reduce_pool_on_idle_os_procs() ->
end).
+should_not_return_broken_process_to_the_pool() ->
+ ?_test(begin
+ config:set("query_server_config",
+ "os_process_soft_limit", "1", false),
+ ok = confirm_config("os_process_soft_limit", "1"),
+
+ config:set("query_server_config",
+ "os_process_limit", "1", false),
+ ok = confirm_config("os_process_limit", "1"),
+
+ DDoc = #doc{
+ id = <<"ddoc1">>,
+ revs = {1, [<<"abc">>]},
+ body = {[
+ {<<"language">>, <<"javascript">>},
+ {<<"views">>, {[
+ {<<"v1">>, {[
+ {<<"map">>, <<"function(doc) {emit(doc.value,1);}">>}
+ ]}}
+ ]}}
+ ]}
+ },
+ meck:reset(couch_os_process),
+
+ ?assertEqual(0, couch_proc_manager:get_proc_count()),
+ ok = couch_query_servers:with_ddoc_proc(DDoc, fun(_) -> ok end),
+ ?assertEqual(0, meck:num_calls(couch_os_process, stop, 1)),
+ ?assertEqual(1, couch_proc_manager:get_proc_count()),
+
+ ?assertError(bad, couch_query_servers:with_ddoc_proc(DDoc, fun(_) ->
+ error(bad)
+ end)),
+ ?assertEqual(1, meck:num_calls(couch_os_process, stop, 1)),
Review comment:
Good idea
##########
File path: src/couch/test/eunit/couchdb_os_proc_pool.erl
##########
@@ -205,6 +208,46 @@ should_reduce_pool_on_idle_os_procs() ->
end).
+should_not_return_broken_process_to_the_pool() ->
+ ?_test(begin
+ config:set("query_server_config",
+ "os_process_soft_limit", "1", false),
+ ok = confirm_config("os_process_soft_limit", "1"),
+
+ config:set("query_server_config",
+ "os_process_limit", "1", false),
+ ok = confirm_config("os_process_limit", "1"),
+
+ DDoc = #doc{
+ id = <<"ddoc1">>,
+ revs = {1, [<<"abc">>]},
+ body = {[
+ {<<"language">>, <<"javascript">>},
+ {<<"views">>, {[
+ {<<"v1">>, {[
+ {<<"map">>, <<"function(doc) {emit(doc.value,1);}">>}
+ ]}}
+ ]}}
+ ]}
+ },
+ meck:reset(couch_os_process),
+
+ ?assertEqual(0, couch_proc_manager:get_proc_count()),
+ ok = couch_query_servers:with_ddoc_proc(DDoc, fun(_) -> ok end),
+ ?assertEqual(0, meck:num_calls(couch_os_process, stop, 1)),
+ ?assertEqual(1, couch_proc_manager:get_proc_count()),
+
+ ?assertError(bad, couch_query_servers:with_ddoc_proc(DDoc, fun(_) ->
+ error(bad)
+ end)),
+ ?assertEqual(1, meck:num_calls(couch_os_process, stop, 1)),
+
+ ok = couch_query_servers:with_ddoc_proc(DDoc, fun(_) -> ok end),
Review comment:
We could just for completeness, though we already asserted that stop was called to kill the one
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org