Skip to content

Commit

Permalink
Treat javascript internal errors as fatal
Browse files Browse the repository at this point in the history
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 apache#4504
  • Loading branch information
nickva committed Mar 29, 2023
1 parent 75ee0f0 commit cd77b07
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 1 deletion.
8 changes: 8 additions & 0 deletions share/server/loop.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down
2 changes: 2 additions & 0 deletions share/server/views.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
38 changes: 37 additions & 1 deletion src/couch/test/eunit/couch_js_tests.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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}
]
}
}.
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit cd77b07

Please sign in to comment.