Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node crashes when writing data on HTTP2 stream that meanwhile terminates #24037

Closed
hermanbanken opened this issue Nov 2, 2018 · 12 comments
Closed
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@hermanbanken
Copy link

hermanbanken commented Nov 2, 2018

  • Version: v8.12.0
  • Platform: OSX, Darwin mymac.local 17.7.0 Darwin Kernel Version 17.7.0: Thu Jun 21 22:53:14 PDT 2018; root:xnu-4570.71.2~1/RELEASE_X86_64 x86_64
  • Subsystem: http2

When performing something along the lines of:

const http2Session = ...;
const outHeaders = { ... };
const req = http2Session.request({ ...outHeaders, [HTTP2_HEADER_PATH]: path });
const body: Buffer[] = [];
let headers: IncomingHttpHeaders = {};
req
      .on("response", (hs) => headers = hs)
      .on("data", (chunk) => body.push(chunk))
      .on("error", (error) => reject(error))
      .on("end", () => resolve({
        headers,
        status: parseInt(headers[HTTP2_HEADER_STATUS], 10),
        body: Buffer.concat(body),
      }));
req.end(Buffer.from("MYDATA"));

and the NGINX server becomes unavailable (shutsdown, GOAWAY, sessions close) sometimes we get the following error:

node[25931]: ../src/tls_wrap.cc:604:virtual int node::TLSWrap::DoWrite(node::WriteWrap *, uv_buf_t *, size_t, uv_stream_t *): Assertion `(ssl_) != (nullptr)' failed.
 1: node::Abort() [/Users/hbanken/.nvm/versions/node/v8.12.0/bin/node]
 2: node::(anonymous namespace)::DomainEnter(node::Environment*, v8::Local<v8::Object>) [/Users/hbanken/.nvm/versions/node/v8.12.0/bin/node]
 3: node::TLSWrap::DoWrite(node::WriteWrap*, uv_buf_t*, unsigned long, uv_stream_s*) [/Users/hbanken/.nvm/versions/node/v8.12.0/bin/node]
 4: node::http2::Http2Session::SendPendingData() [/Users/hbanken/.nvm/versions/node/v8.12.0/bin/node]
 5: node::http2::Http2Stream::RstStream(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/hbanken/.nvm/versions/node/v8.12.0/bin/node]
 6: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/hbanken/.nvm/versions/node/v8.12.0/bin/node]
 7: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/hbanken/.nvm/versions/node/v8.12.0/bin/node]
 8: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/hbanken/.nvm/versions/node/v8.12.0/bin/node]
 9: 0x3671961042fd
10: 0x3671961bd1d6

Expected behavior:

Native error is caught and converted in something NodeJS can handle instead of crashing the whole process.

@hermanbanken
Copy link
Author

Seems to be resolved by 0c25cdf #18987 and error might be a duplicate of #18973.

@hermanbanken
Copy link
Author

Also #19002 seems to be related.

@hermanbanken
Copy link
Author

Question remaining: can we backport to 8.12 or don't we because it is experimental in 8.12?

@richardlau
Copy link
Member

Quite a few http2 commits have been backported for the proposed 8.13.0: #23974

@hermanbanken
Copy link
Author

But not yet #19002 or #18973, right?

@richardlau
Copy link
Member

I believe #19002 was included in v8.11.2 via c976cb5be5 as part of #20456.

@hermanbanken
Copy link
Author

That is unfortunate, as it has npm 5.6: we need at least 8.12 (because npm 5.7 with npm ci) and can't switch to Node 10.13 as there is an issue related to OpenSSL (1.1.0?) causing a ssl3_GET_RECORD:wrong version number on the client.

@richardlau
Copy link
Member

Not sure I follow you, 8.12 should contain all of the commits in 8.11.2 (unless something was reverted, but I don't believe that was the case).

@hermanbanken
Copy link
Author

hermanbanken commented Nov 2, 2018 via email

@sam-github
Copy link
Contributor

We could do an 8.11.2 by the rules of semver... but we don't do that. Since 8.12 has no incompatible changes from 8.11.2, we would ask anyone having a problem with 8.11.x to upgrade to 8.12.x (or later).

@Trott Trott added the http2 Issues or PRs related to the http2 subsystem. label Nov 4, 2018
@Trott
Copy link
Member

Trott commented Nov 19, 2018

@nodejs/http2

@addaleax
Copy link
Member

I’m closing this as v8.x is no longer supported … sorry if that wasn’t the outcome everyone was hoping for, but there aren’t going to be any more v8.x releases at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants