[couchdb] branch do_not_return_bad_os_processes_into_the_pool created (now 3c51d95)

[va...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

vatamane pushed a change to branch do_not_return_bad_os_processes_into_the_pool
in repository https://gitbox.apache.org/repos/asf/couchdb.git.


      at 3c51d95  Do not return broken processes to the query process pool

This branch includes the following new commits:

     new 3c51d95  Do not return broken processes to the query process pool

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.

[couchdb] 01/01: Do not return broken processes to the query process pool

[va...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

vatamane pushed a commit to branch do_not_return_bad_os_processes_into_the_pool
in repository https://gitbox.apache.org/repos/asf/couchdb.git

commit 3c51d954a0517cb21b6f36f98838b840f63746e2
Author: Nick Vatamaniuc <va...@apache.org>
AuthorDate: Fri Jan 15 12:23:50 2021 -0500

    Do not return broken processes to the query process pool
    
    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
---
 src/couch/src/couch_query_servers.erl         | 11 +++++--
 src/couch/test/eunit/couchdb_os_proc_pool.erl | 45 ++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/src/couch/src/couch_query_servers.erl b/src/couch/src/couch_query_servers.erl
index 6649df3..c6d7134 100644
--- a/src/couch/src/couch_query_servers.erl
+++ b/src/couch/src/couch_query_servers.erl
@@ -523,9 +523,14 @@ with_ddoc_proc(#doc{id=DDocId,revs={Start, [DiskRev|_]}}=DDoc, Fun) ->
     Rev = couch_doc:rev_to_str({Start, DiskRev}),
     DDocKey = {DDocId, Rev},
     Proc = get_ddoc_process(DDoc, DDocKey),
-    try Fun({Proc, DDocId})
-    after
-        ok = ret_os_process(Proc)
+    try Fun({Proc, DDocId}) of
+        Resp ->
+            ok = ret_os_process(Proc),
+            Resp
+    catch Tag:Err ->
+        Stack = erlang:get_stacktrace(),
+        catch proc_stop(Proc),
+        erlang:raise(Tag, Err, Stack)
     end.
 
 proc_prompt(Proc, Args) ->
diff --git a/src/couch/test/eunit/couchdb_os_proc_pool.erl b/src/couch/test/eunit/couchdb_os_proc_pool.erl
index 69f8051..8f732b9 100644
--- a/src/couch/test/eunit/couchdb_os_proc_pool.erl
+++ b/src/couch/test/eunit/couchdb_os_proc_pool.erl
@@ -20,9 +20,11 @@
 
 setup() ->
     ok = couch_proc_manager:reload(),
+    meck:new(couch_os_process, [passthrough]),
     ok = setup_config().
 
 teardown(_) ->
+    meck:unload(),
     ok.
 
 os_proc_pool_test_() ->
@@ -39,7 +41,8 @@ os_proc_pool_test_() ->
                     should_free_slot_on_proc_unexpected_exit(),
                     should_reuse_known_proc(),
 %                    should_process_waiting_queue_as_fifo(),
-                    should_reduce_pool_on_idle_os_procs()
+                    should_reduce_pool_on_idle_os_procs(),
+                    should_not_return_broken_process_to_the_pool()
                 ]
             }
         }
@@ -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),
+        ?assertEqual(1, meck:num_calls(couch_os_process, stop, 1)),
+        ?assertEqual(1, couch_proc_manager:get_proc_count())
+    end).
+
+
 setup_config() ->
     config:set("native_query_servers", "enable_erlang_query_server", "true", false),
     config:set("query_server_config", "os_process_limit", "3", false),