-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
the Stream is closed if app pushes unused assets #20824
Comments
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 |
@apapirovski 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;
}); |
It didnt helped - after some refreshes here is another error:
Please 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:
|
Sounds like there are potentially issues in our C++ pipe code. ping @addaleax |
@budarin I could take a look into this, but could you bring me up to speed by sharing the complete relevant code for the server script? Or is the |
here is the repository for experiments and testing http2 project |
@budarin Umm, I uncommented the two lines and reloaded, the page loads up as expected. That said, I'm using chrome to connect. Hope that's not an issue? |
|
I have fixed readme for the repo |
@budarin had been holding the keys for a minute or two, noticed nothing, realized I was using Node 8. Switched to Node 10, didn't take me more than a few seconds to get a crash. Maybe this was broken fairly recently in some related change? |
FWIW: It throws on my computer with: respondToStreamError Error [ERR_HTTP2_STREAM_ERROR]: Stream closed with error code NGHTTP2_REFUSED_STREAM
at ServerHttp2Stream._destroy (internal/http2/core.js:1871:13)
at ServerHttp2Stream.destroy (internal/streams/destroy.js:32:8)
at ServerHttp2Stream.[maybe-destroy] (internal/http2/core.js:1887:12)
at Http2Stream.onStreamClose [as onstreamclose] (internal/http2/core.js:346:26)
respondToStreamError Error [ERR_HTTP2_STREAM_ERROR]: Stream closed with error code NGHTTP2_REFUSED_STREAM
at ServerHttp2Stream._destroy (internal/http2/core.js:1871:13)
at ServerHttp2Stream.destroy (internal/streams/destroy.js:32:8)
at ServerHttp2Stream.[maybe-destroy] (internal/http2/core.js:1887:12)
at Http2Stream.onStreamClose [as onstreamclose] (internal/http2/core.js:346:26)
respondToStreamError Error [ERR_HTTP2_STREAM_ERROR]: Stream closed with error code NGHTTP2_REFUSED_STREAM
at ServerHttp2Stream._destroy (internal/http2/core.js:1871:13)
at ServerHttp2Stream.destroy (internal/streams/destroy.js:32:8)
at ServerHttp2Stream.[maybe-destroy] (internal/http2/core.js:1887:12)
at Http2Stream.onStreamClose [as onstreamclose] (internal/http2/core.js:346:26)
respondToStreamError Error [ERR_HTTP2_STREAM_ERROR]: Stream closed with error code NGHTTP2_REFUSED_STREAM
at ServerHttp2Stream._destroy (internal/http2/core.js:1871:13)
at ServerHttp2Stream.destroy (internal/streams/destroy.js:32:8)
at ServerHttp2Stream.[maybe-destroy] (internal/http2/core.js:1887:12)
at Http2Stream.onStreamClose [as onstreamclose] (internal/http2/core.js:346:26)
node[4367]: ../src/node_file.cc:215:MaybeLocal<v8::Promise> node::fs::FileHandle::ClosePromise(): Assertion `!reading_' failed.
1: node::Abort() [/Users/ryzokuken/.nvm/versions/node/v10.1.0/bin/node]
2: node::InternalCallbackScope::~InternalCallbackScope() [/Users/ryzokuken/.nvm/versions/node/v10.1.0/bin/node]
3: void node::StreamBase::AddMethods<node::fs::FileHandle>(node::Environment*, v8::Local<v8::FunctionTemplate>, int) [/Users/ryzokuken/.nvm/versions/node/v10.1.0/bin/node]
4: node::fs::FileHandle::Close(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/ryzokuken/.nvm/versions/node/v10.1.0/bin/node]
5: v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) [/Users/ryzokuken/.nvm/versions/node/v10.1.0/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) [/Users/ryzokuken/.nvm/versions/node/v10.1.0/bin/node]
7: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/ryzokuken/.nvm/versions/node/v10.1.0/bin/node]
8: 0x68d9810427d |
@ryzokuken It's because it pipes the file entirely through C++ now. I pinged @addaleax because she worked on that code. I don't know if anyone else knows that code well enough. I unfortunately won't have the time to spend learning & debugging it in the next few weeks. Feel free to take a stab at it. The PR that introduced that code was here: #18936 |
I began try http2 recently with Node version 10.1.0 - so I can not say anything ( |
Yeah, this is on my radar – I’ve been able to reproduce it locally, just haven’t gotten around to anything yet. This should not be too hard to fix, though. It seems like there are race conditions between the On the (Happy to guide you through this, @ryzokuken, if you’re interested and okay with me having somewhat limited bandwith to spend on it) |
@ryzokuken but with those two lines commented out - it takes much more time to get the error |
@budarin maybe that's just because the amount of requests is low enough? Maybe it reaches some sort of critical mass once two more requests are performed per reload. I'd be trying to hack up a simpler and smaller test case in the morning, let's see how it goes. |
@budarin Okay, I think I'm done. @budarin @apapirovski @addaleax check out https://gist.github.com/ryzokuken/71392a6cc0a962b5c0ea0662e8a3ae6a |
@ryzokuken I’ve just tried that, but I couldn’t really get it to crash or to run – it always ends up on
😕 |
I think this got fixed but not in a release yet? I could be wrong. Perhaps @budarin can verify when we cut the next release (10.4.0?). |
@apapirovski I managed to get a reproduction on master, so, unfortunately no. |
Hey, I can confirm this, testing it with latest
See my MCVE gist My tests show:
|
I also put all frames Chrome is logging to the gist. In the gist you can see the last Refresh, before it crashed and I think |
I try to create a red test, but unfortunately I can't find a way with the client to cancel a server push? I can only listen on it docs |
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: I'm wondering if 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? |
@DaAitch I think the check is correct, it’s rather that the 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.
|
I have this problem too if add this code
its work, but if i often reload page then node crash
|
I'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... |
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
The text was updated successfully, but these errors were encountered: