http2: exception on stream destroy

[fcicq]

Version: 8.8.1
Platform: 64bit linux
Subsystem: http2

I'm trying to write a simple nghttpx clone, so you see stream.respond & pipe there.
I was finding the right way to destroy the httpres and the stream.
I'm sorry that I don't know how to reliable reproduce this.

exceptions found:

internal/http2/core.js:1355
const handle = session[kHandle];
                          ^

TypeError: Cannot read property 'Symbol(handle)' of undefined
    at ServerHttp2Stream._write (internal/http2/core.js:1355:27)
    at doWrite (_stream_writable.js:387:12)
    at clearBuffer (_stream_writable.js:514:7)
    at onwrite (_stream_writable.js:439:7)
    at WriteWrap.afterDoStreamWrite [as oncomplete] (internal/http2/core.js:1157:9)
    at Immediate.finishStreamDestroy [as _onImmediate] (internal/http2/core.js:1534:12)
    at runCallback (timers.js:789:20)
    at tryOnImmediate (timers.js:747:5)
    at processImmediate [as _immediateCallback] (timers.js:718:5)

and this
(maybe caused by stream.rstStream(http2.constants.NGHTTP2_CONNECT_ERROR);)

internal/http2/core.js:1377
    const handle = session[kHandle];
                          ^

TypeError: Cannot read property 'Symbol(handle)' of undefined
    at ServerHttp2Stream._writev (internal/http2/core.js:1377:27)
    at doWrite (_stream_writable.js:385:12)
    at clearBuffer (_stream_writable.js:491:5)
    at onwrite (_stream_writable.js:439:7)
    at WriteWrap.afterDoStreamWrite [as oncomplete] (internal/http2/core.js:1157:9)
    at Immediate.finishStreamDestroy [as _onImmediate] (internal/http2/core.js:1534:12)
    at runCallback (timers.js:789:20)
    at tryOnImmediate (timers.js:747:5)
    at processImmediate [as _immediateCallback] (timers.js:718:5)

snippet:

const server = http2.createServer();

server.on('stream', (stream, headers) => {
 // ... here is the request
var httpreq = http.request(parsed, function(httpres) {
    httpres.headers[':status'] = httpres.statusCode;
    delete httpres.headers['connection'];
    delete httpres.headers['keep-alive'];
    delete httpres.headers['proxy-connection'];
    delete httpres.headers['host'];
    delete httpres.headers['upgrade'];
    delete httpres.headers['transfer-encoding'];
    // Note: this is issue 16452
    if (httpres.headers['set-cookie'] && httpres.headers['set-cookie'].length ==
 1) {
      httpres.headers['set-cookie'] = httpres.headers['set-cookie'][0];
    }
    stream.respond(httpres.headers);
    httpres.pipe(stream, {end: true});
});
  httpreq.on('error', (error) => {
    if (!stream.headersSent && !stream.destroyed) {
      stream.respond({':status': 502});
      stream.end('502 Error');
    } else if (!stream.destroyed) {
      httpreq.abort();
      stream.destroy();
    }
  });
  stream.on('error', (error) => {
    httpreq.abort();
  });
  stream.on('aborted', () => { httpreq.abort() });
  stream.on('streamClosed', () => { httpreq.abort() });
  stream.pipe(httpreq, {end: true});
}

[jasnell]

Will investigate! Thank you! :-)

ping @mcollina @apapirovski @sebdeckers

[apapirovski]

Hmm, is this happening when you stream.destroy from the error handler? You mention stream.rstStream(http2.constants.NGHTTP2_CONNECT_ERROR); but I don't see it anywhere in the snippet above.

Based on the stack trace, it seems like you might be destroying it while the pipe is still ongoing but I can't quite tell since I can't run this code as is. In general you should always explicitly end broken streams that are part of a pipe.

If I'm right, I wonder if there's anything we can do better here.

[fcicq]

@apapirovski thanks for the comment. previously I use stream.rstStream() directly, without stream.destory().

actually I find the pipe() is quite hard to get correct, but it is nearly the only way to handle back-pressure, so I cant use on('data', (d) => {stream.write(d)}).
I don't know the correct way to notify the other side about the event (unpipe, finish, close, which one? what to do? end or unpipe or abort or destroy?).

just found stream can be .abort(), will try.

[fcicq]

new code under testing.

  httpreq.on('error', (error) => {
    httpreq.abort();
    if (!stream.headersSent && !stream.aborted && !stream.destroyed) {
      stream.respond({':status': 502});
      stream.end('502 Error');
    } else if (!stream.aborted && !stream.destroyed) {
      stream.rstStream(http2.constants.NGHTTP2_STREAM_CLOSED);
    }
  });

[fcicq]

looks resolved, needs a little more time to confirm and close.

so the problem may be one cant use stream.destroy() on "aborted" stream, double destroy problem?

[fcicq]

Sorry, same exception (one of the two) still exists. happen even less frequently by the 3rd version of error handling code.

  httpreq.on('error', (error) => {
    httpreq.abort();
    var stream_alive = !stream.aborted && !stream.destroyed && stream.session;
    if (!stream.headersSent && stream_alive) {
      stream.respond({':status': 502});
      stream.end('502 Error');
    } else if (stream_alive) {
      stream.rstStream(http2.constants.NGHTTP2_CONNECT_ERROR);
    }
  });

the timing is related with my broadband pppoe force reconnection kickout happens every 24h,
so I suspect it may related with socket failure & http2session closing.

[fcicq]

I guess it is fixed by #16525, no more exceptions on node 9.0.0, closing.