From cd77b07cfbcd0f4c290d98bf15e65c0130ed7d4b Mon Sep 17 00:00:00 2001 From: Nick Vatamaniuc Date: Wed, 29 Mar 2023 15:25:18 -0400 Subject: [PATCH] Treat javascript internal errors as fatal Spidermonkey sometimes throws an `InternalError` when exceeding memory limits, when normally we'd expect it to crash or exit with a non-0 exit code. Because we trap exceptions, and continue emitting rows, it is possible for users views to randomly miss indexed rows based on whether GC had run or not, other internal runtime state which may have been consuming more or less memory until that time. To prevent the view continuing processing documents, and randomly dropping emitted rows, depending on memory pressure in the JS runtime at the time, choose to treat Internal errors as fatal. After an InternalError is raised we expect the process to exit just like it would during OOM. Add a test to assert this happens. Fix https://github.com/apache/couchdb/issues/4504 --- share/server/loop.js | 8 ++++++ share/server/views.js | 2 ++ src/couch/test/eunit/couch_js_tests.erl | 38 ++++++++++++++++++++++++- 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/share/server/loop.js b/share/server/loop.js index 70a143a45f9..3ab303c214c 100644 --- a/share/server/loop.js +++ b/share/server/loop.js @@ -130,6 +130,14 @@ var Loop = function() { quit(-1); } else if (type == "error") { respond(e); + } else if (e.name == "InternalError") { + // If the internal error is caught by handleViewError it will be + // re-thrown as a ["fatal", ...] error, and we already handle that above. + // Here we handle the case when the error is thrown outside of + // handleViewError, for instance when serializing the rows to be sent + // back to the user + respond(["error", e.name, e.message]); + quit(-1); } else if (e.error && e.reason) { // compatibility with old error format respond(["error", e.error, e.reason]); diff --git a/share/server/views.js b/share/server/views.js index 32d65e45756..57cdaf3a9c0 100644 --- a/share/server/views.js +++ b/share/server/views.js @@ -73,6 +73,8 @@ var Views = (function() { // Throwing errors of the form ["fatal","error_key","reason"] // will kill the OS process. This is not normally what you want. throw(err); + } else if (err.name == "InternalError") { + throw(["fatal", err.name, err.message]); } var message = "function raised exception " + err.toSource(); if (doc) message += " with doc._id " + doc._id; diff --git a/src/couch/test/eunit/couch_js_tests.erl b/src/couch/test/eunit/couch_js_tests.erl index 789b3632170..afd42bb72bd 100644 --- a/src/couch/test/eunit/couch_js_tests.erl +++ b/src/couch/test/eunit/couch_js_tests.erl @@ -28,7 +28,8 @@ couch_js_test_() -> fun should_roundtrip_modified_utf8/0, fun should_replace_broken_utf16/0, fun should_allow_js_string_mutations/0, - {timeout, 60000, fun should_exit_on_oom/0} + {timeout, 60000, fun should_exit_on_oom/0}, + {timeout, 60000, fun should_exit_on_internal_error/0} ] } }. @@ -236,6 +237,41 @@ should_exit_on_oom() -> true = couch_query_servers:proc_prompt(Proc, [<<"add_fun">>, Src]), trigger_oom(Proc). +%% erlfmt-ignore +should_exit_on_internal_error() -> + % A different way to trigger OOM which previously used to + % throw an InternalError on SM. Check that we still exit on that + % type of error + Src = <<" + function(doc) { + function mkstr(l) { + var r = 'r'; + while (r.length < l) {r = r + r;} + return r; + } + var s = mkstr(96*1024*1024); + var a = [s,s,s,s,s,s,s,s]; + var j = JSON.stringify(a); + emit(42, j.length);} + ">>, + Proc = couch_query_servers:get_os_process(<<"javascript">>), + true = couch_query_servers:proc_prompt(Proc, [<<"reset">>]), + true = couch_query_servers:proc_prompt(Proc, [<<"add_fun">>, Src]), + Doc = {[]}, + try + couch_query_servers:proc_prompt(Proc, [<<"map_doc">>, Doc]) + catch + % Expect either an internal error thrown if it catches it and + % emits an error log before dying + throw:{<<"InternalError">>, _} -> + ok; + % It may fail and just exit the process. That's expected as well + throw:{os_process_error, _} -> + ok + end, + % Expect the process to be dead + ?assertThrow({os_process_error, _}, couch_query_servers:proc_prompt(Proc, [<<"reset">>])). + trigger_oom(Proc) -> Status = try