@grpc/grpc-js: SIGABRT with worker threads (Assertion `onread->IsFunction()' failed.)

[Prinzhorn]

Problem description

I kept getting SIGABRT every like 100 times I closed my Electron app. I was able to narrow this down to the @grpc/grpc-js package. It happens when a worker thread is terminated just before a @grpc/grpc-js server tries to read on the stream.

It is potentially caused by the underlying http/2 module. But with just http/2 it'll result in a "FATAL ERROR:" (nodejs/node#38418 (comment)), which might be completely unrelated or maybe it masks the SIGABRT. Or the SIGABRT is caused by how @grpc/grpc-js is using http/2 (e.g. where did onread go?) and not related to http/2 itself at all.

STREAM 22227: ondata
heartbeat request 1652
STREAM 22227: dest.write true
STREAM 22227: maybeReadMore read 0
STREAM 22227: read 0
STREAM 22227: need readable true
STREAM 22227: length less than watermark true
STREAM 22227: do read
TIMER 22227: process timer lists 4637
TIMER 22227: timeout callback 2000
WORKER 22227: [0] terminates Worker with ID 1
node[22227]: ../src/stream_base.cc:351:v8::MaybeLocal<v8::Value> node::StreamBase::CallJSOnreadMethod(ssize_t, v8::Local<v8::ArrayBuffer>, size_t, node::StreamBase::StreamBaseJSChecks): Assertion `onread->IsFunction()' failed.
TIMER 22227: 2000 list empty
 1: 0xa222f0 node::Abort() [node]
 2: 0xa2236e  [node]
 3: 0xaedd0a  [node]
 4: 0xa4380e node::http2::Http2StreamListener::OnStreamRead(long, uv_buf_t const&) [node]
 5: 0xa4f66c node::http2::Http2Session::OnDataChunkReceived(nghttp2_session*, unsigned char, int, unsigned char const*, unsigned long, void*) [node]
 6: 0x18a3d0d nghttp2_session_mem_recv [node]
 7: 0xa4eb09 node::http2::Http2Session::ConsumeHTTP2Data() [node]
 8: 0xa4ee5e node::http2::Http2Session::OnStreamRead(long, uv_buf_t const&) [node]
 9: 0xafa168 node::LibuvStreamWrap::OnUvRead(long, uv_buf_t const*) [node]
10: 0x13a8bf7  [node]
11: 0x13a95b0  [node]
12: 0x13b0095  [node]
13: 0x139dc38 uv_run [node]
14: 0xada493 node::worker::Worker::Run() [node]
15: 0xadaf18  [node]
16: 0x7fa0cf30f609  [/lib/x86_64-linux-gnu/libpthread.so.0]
17: 0x7fa0cf236293 clone [/lib/x86_64-linux-gnu/libc.so.6]
Aborted (core dumped)

Reproduction steps

Repo is here https://github.com/Prinzhorn/electron-sigabrt
Please note that it does no longer require Electron at all (as the name suggests), it was stripped down.

Also HEAD does not use @grpc/grpc-js (because I stripped it away and uncovered the bug in http/2 module), you need to check out commit 45caa541d9d16bbeb1b7a9e31ebbd59e61381b7d

What the repo does is start a grpc server inside a worker thread and then bombard it with requests. The worker is terminated after 2s and this will reliable cause the whole process to SIGABRT if it happens at the right time.

git clone git@github.com:Prinzhorn/electron-sigabrt.git
cd electron-sigabrt/
git checkout 45caa541d9d16bbeb1b7a9e31ebbd59e61381b7d
npm i

# In two terminals (server will terminate after 2s automatically)
npm run server
npm run client

vokoscreen-2021-05-25_18-21-37.mp4

Environment

• OS name, version and architecture: Linux alex-desktop 5.8.0-53-generic Upgrade npm versions for every version of node in test scripts #60~20.04.1-Ubuntu SMP Thu May 6 09:52:46 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
• Node version v14.17.0 and v16.2.0
• Node installation method [e.g. nvm] nvm
• Package name and version [e.g. gRPC@1.12.0] @grpc/grpc-js 1.3.2 (the repo's package.json still points to 1.3.0, it repros with 1.3.2 as well you can update the package.json)

[murgatroid99]

From my point of view, if it is possible to use a JavaScript API in such a way that it causes the interpreter to crash, that is always a bug with Node itself. The fact that this specifically happens under heavy load makes that even more likely; grpc-js does not behave any differently under heavy load, so I assume this is a problem with some internal data structure.

[Prinzhorn]

From my point of view, if it is possible to use a JavaScript API in such a way that it causes the interpreter to crash, that is always a bug with Node itself

I guess so, if grpc-js is only using public APIs. As long as my repro requires grpc-js the Node.js team will probably not take it seriously. And with the vanilla http2 client/server (with the most basic hello world from the docs) I've surfaced a seemingly unrelated race condition. Maybe someone with knowledge of how grpc-js internally uses the http2 module could update HEAD of https://github.com/Prinzhorn/electron-sigabrt to repro the SIGABRT instead of the FATAL ERROR it's causing.

The fact that this specifically happens under heavy load makes that even more likely

The load only increases the chances of it happening, since this is a race condition with timing the worker.terminate() call. It originally happened like every 100 times I closed my Electron app (which terminates the worker) because a gRPC call is happening every 1s, so it was just rare for me to hit the correct time.

[murgatroid99]

Looking at the linked Node issue again, I think including the two different "abort" errors in that one issue confused things, and I don't think they're dismissing the original bug that happened with grpc-js just because another library is using the API. I think they were just saying that the second error that you found using the vanilla http2 APIs is different than the original bug you filed.

[murgatroid99]

I don't think this is a gRPC bug.