Repository navigation
the Stream is closed if app pushes unused assets #20824
Description
Activity
- changed the title
[-]Stream closes when app pushes unused assets[/-][+]the Stream is closed if app pushes unused assets[/+]on May 18, 2018 Here is the demo project
try to uncomment these lines:
// pushAsset(stream, jsFile1); // pushAsset(stream, jsFile2);
and make some quick page refreshes - it makes the error.
/cc @nodejs/http2
This seems as expected to me. Add an error handler that ignores
NGHTTP2_REFUSED_STREAMerrors. See the spec: https://http2.github.io/http2-spec/#rfc.section.7@apapirovski
I'm sorry, could you provide the code, please.I don't quite understand what you mean.
Sorry, should've been clearer, you can use something like this to make sure those errors don't take down your app:
pushStream.on('error', (err) => { const isRefusedStream = err.code === 'ERR_HTTP2_STREAM_ERROR' && pushStream.rstCode === NGHTTP2_REFUSED_STREAM; if (!isRefusedStream) throw err; });
Reacted by Roger PadillaReacted by Roger Padilla and himanshu-saraswatIt didnt helped - after some refreshes here is another error:
npm[10144]: src\node_file.cc:215: Assertion `!reading_' failed. 1: node::DecodeWrite 2: node::DecodeWrite 3: uv_loop_fork 4: uv_loop_fork 5: v8::internal::interpreter::BytecodeDecoder::Decode 6: v8::internal::RegExpImpl::Exec 7: v8::internal::RegExpImpl::Exec 8: v8::internal::RegExpImpl::Exec 9: 000000B116304281Please review the code:
const pushAsset = (stream, file) => { const filePath = path.resolve(path.join(serverRoot, file.filePath)); stream.pushStream({ [HTTP2_HEADER_PATH]: file.path }, { parent: stream.id }, (err, pushStream) => { if (!err) { console.log(">> Pushing:", file.path); pushStream.on('error', (err) => { console.log('pushStream.on error', err); const isRefusedStream = err.code === 'ERR_HTTP2_STREAM_ERROR' && pushStream.rstCode === NGHTTP2_REFUSED_STREAM; if (isRefusedStream) { return; } respondToStreamError(err, pushStream); }); pushStream.respondWithFile(filePath, file.headers); } if (err) { console.log(">> Pushing error:", err); } }); };
And even not pushing unnessesary assets, but quickly refresh the page - you will get the error:
npm[5912]: src\node_http2.cc:1934: Assertion `!this->IsDestroyed()' failed. 1: node::DecodeWrite 2: node::DecodeWrite 3: ENGINE_get_ctrl_function 4: DH_get0_engine 5: std::basic_ostream<char,std::char_traits<char> >::basic_ostream<char,std::char_traits<char> > 6: uv_loop_fork 7: uv_fs_get_statbuf 8: uv_dlerror 9: node::CreatePlatform 10: node::CreatePlatform 11: node::Start 12: v8::SnapshotCreator::`default constructor closure' 13: v8::internal::AsmJsScanner::IsNumberStart 14: BaseThreadInitThunk 15: RtlUserThreadStartSounds like there are potentially issues in our C++ pipe code.
ping @addaleax
here is the repository for experiments and testing http2 project
The two JavaScript files don't show up here, though? I think that's expected anyway.
16 remaining items
I think this would work:
'use strict'; const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); const http2 = require('http2'); const net = require('net'); const { HTTP2_HEADER_CONTENT_TYPE } = http2.constants; const server = http2.createServer(); server.on('stream', common.mustCall((stream) => { stream.respondWithFile(process.execPath, { [HTTP2_HEADER_CONTENT_TYPE]: 'application/octet-stream' }); })); server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); req.on('response', common.mustCall(() => {})); req.on('data', common.mustCall(() => { net.Socket.prototype.destroy.call(client.socket); server.close(); })); req.end(); }));
It only creates one of the failure modes, though.
@addaleax you're right it failes:
node[9901]: ../src/node_file.cc:220:v8::MaybeLocal<v8::Promise> node::fs::FileHandle::ClosePromise(): Assertion '!reading_' failed.I'm wondering if
CHECK(!reading_);is correct inFileHandle::ClosePromise, sinceAfterClosewill handle aFileHandlebeing in reading state.void FileHandle::AfterClose() { closing_ = false; closed_ = true; if (reading_ && !persistent().IsEmpty()) EmitRead(UV_EOF); }
And how do we get this second error I was talking about?
node[4683]: ../src/node_http2.cc:1966:virtual int node::http2::Http2Stream::DoWrite(node::WriteWrap*, uv_buf_t*, size_t, uv_stream_t*): Assertion '!this->IsDestroyed()' failed.@DaAitch I think the check is correct, it’s rather that the
StreamPipeimplementation shouldn’t keep reading if there was an error on the writable side. This is a patch that fixes this problem, but it’s (obviously?) not an ideal solution:diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc index bfe7d4297257..1d80592b3e75 100644 --- a/src/stream_pipe.cc +++ b/src/stream_pipe.cc @@ -161,2 +161,3 @@ void StreamPipe::WritableListener::OnStreamAfterWrite(WriteWrap* w, pipe->ShutdownWritable(); + pipe->source()->ReadStop(); pipe->Unpipe(); @@ -168,2 +169,3 @@ void StreamPipe::WritableListener::OnStreamAfterWrite(WriteWrap* w, StreamListener* prev = previous_listener_; + pipe->source()->ReadStop(); pipe->Unpipe(); @@ -179,2 +181,3 @@ void StreamPipe::WritableListener::OnStreamAfterShutdown(ShutdownWrap* w, StreamListener* prev = previous_listener_; + pipe->source()->ReadStop(); pipe->Unpipe(); @@ -193,2 +196,3 @@ void StreamPipe::WritableListener::OnStreamDestroy() { pipe->is_eof_ = true; + pipe->source()->ReadStop(); pipe->Unpipe(); @@ -238,2 +242,3 @@ void StreamPipe::Unpipe(const FunctionCallbackInfo<Value>& args) { ASSIGN_OR_RETURN_UNWRAP(&pipe, args.Holder()); + pipe->source()->ReadStop(); pipe->Unpipe();
That second error is probably coming from a similar condition as the one from that test, but with different timing properties (but I’m not sure about that).
@addaleax the patch you provided gives me a green test, but segfaults in my manual test.
edited: I only pasted SIGPIPE, forgot to continue to SIGSEGV.
(gdb) run ../http2/examples/src/index.js file Starting program: /home/aitch/Code/node-fork/node_g ../http2/examples/src/index.js file [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". [New Thread 0x7ffff6b42700 (LWP 19087)] [New Thread 0x7ffff6341700 (LWP 19088)] [New Thread 0x7ffff5b40700 (LWP 19089)] [New Thread 0x7ffff533f700 (LWP 19090)] [New Thread 0x7ffff7ff6700 (LWP 19091)] running with push: file (node:19083) ExperimentalWarning: The http2 module is an experimental API. Thread 1 "node_g" received signal SIGPIPE, Broken pipe. 0x00007ffff6f1d4bd in write () at ../sysdeps/unix/syscall-template.S:84 84 ../sysdeps/unix/syscall-template.S: No such file or directory. (gdb) continue Continuing. Thread 1 "node_g" received signal SIGPIPE, Broken pipe. 0x00007ffff6f1d4bd in write () at ../sysdeps/unix/syscall-template.S:84 84 in ../sysdeps/unix/syscall-template.S (gdb) continue Continuing. respond / push allowed push stream respond /index.css [New Thread 0x7ffff4b3e700 (LWP 19136)] [New Thread 0x7fffe7fff700 (LWP 19137)] [New Thread 0x7fffe77fe700 (LWP 19138)] [New Thread 0x7fffe6ffd700 (LWP 19140)] respond / stream closed respond /index.css Thread 1 "node_g" received signal SIGSEGV, Segmentation fault. 0x00000000018f461c in node::StreamPipe::WritableListener::OnStreamAfterWrite (this=0x4213698, w=0x42136f0, status=0) at ../src/stream_pipe.cc:162 162 pipe->source()->ReadStop();Reacted by Anna HenningsenI have this problem too
if add this code
pushStream.on('error', (err) => { const isRefusedStream = err.code === 'ERR_HTTP2_STREAM_ERROR' && pushStream.rstCode === NGHTTP2_REFUSED_STREAM; if (!isRefusedStream) throw err; });its work, but if i often reload page then node crash
node[17568]: ../src/node_file.cc:220:MaybeLocal<v8::Promise> node::fs::FileHandle::ClosePromise(): Assertion `!reading_' failed. 1: node::Abort() [/usr/local/bin/node] 2: node::MakeCallback(v8::Isolate*, v8::Local<v8::Object>, char const*, int, v8::Local<v8::Value>*, node::async_context) [/usr/local/bin/node] 3: node::fs::FileHandle::ClosePromise() [/usr/local/bin/node] 4: node::fs::FileHandle::Close(v8::FunctionCallbackInfo<v8::Value> const&) [/usr/local/bin/node] 5: v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) [/usr/local/bin/node] 6: 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) [/usr/local/bin/node] 7: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/usr/local/bin/node] 8: 0x138017d041bd Abort trap: 6I'm going to spend some time with this today, hopefully I'll get to the point where I can make a PR.
@addaleax Are you still able to reproduce? Because I tried against 10.5.0 with all 3 examples in this thread and I can't get it to fail...
- added a commit that references this issue
on Jul 15, 2018 - added a commit that references this issue
on Jul 16, 2018 - added a commit that references this issue
on Jul 27, 2026
If push some assets which will not be used with browser - stream closes on the second request
Page requires only
<script src="script.js"></script>, but if to push something that is not using on the page - the next page refresh will close the http2 streamThe error:
Declaration of extra assets should not crush the tream