Http2Stream emit connection related 'error' after receiving all data of stream.

[sogaani]

The following conditions cause an error event on http2stream, even though http2stream received all data and RST_STREAM from a client.

1. http2stream is not destroyed(does not consume all data)
2. goaway frame is not received
3. socket error happens
4. http2stream received all data and RST_STREAM

Socket error without goaway frame causes a http2session.destroy(err).

node/lib/internal/http2/core.js

Lines 2678 to 2688 in 81bc7b3

	function socketOnError(error) {
	const session = this[kSession];
	if (session !== undefined) {
	// We can ignore ECONNRESET after GOAWAY was received as there's nothing
	// we can do and the other side is fully within its rights to do so.
	if (error.code === 'ECONNRESET' && session[kState].goawayCode !== null)
	return session.destroy();
	debugSessionObj(this, 'socket error [%s]', error.message);
	session.destroy(error);
	}
	}

http2session.destroy(err) propagate error to http2stream.destroy(err).

node/lib/internal/http2/core.js

Lines 1298 to 1323 in 81bc7b3

	destroy(error = NGHTTP2_NO_ERROR, code) {
	if (this.destroyed)
	return;
	debugSessionObj(this, 'destroying');
	if (typeof error === 'number') {
	code = error;
	error =
	code !== NGHTTP2_NO_ERROR ?
	new ERR_HTTP2_SESSION_ERROR(code) : undefined;
	}
	if (code === undefined && error != null)
	code = NGHTTP2_INTERNAL_ERROR;
	const state = this[kState];
	state.flags |= SESSION_FLAGS_DESTROYED;
	state.destroyCode = code;
	// Clear timeout and remove timeout listeners
	this.setTimeout(0);
	this.removeAllListeners('timeout');
	// Destroy any pending and open streams
	const cancel = new ERR_HTTP2_STREAM_CANCEL(error);
	state.pendingStreams.forEach((stream) => stream.destroy(cancel));
	state.streams.forEach((stream) => stream.destroy(error));

I thought that http2stream got RST_STREAM means that there is no error about processing data. So, I thought if http2stream is closed by RST_STREAM, connection error should not cause http2stream error.

The codes to reproduce.

'use strict';

const common = require('../common');
const fixtures = require('../common/fixtures');
const http2 = require('http2');
const fs = require('fs');
const net = require('net');

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();
const loc = fixtures.path('person-large.jpg');

const server = http2.createServer();
let session_;
server.on('session', (session)=>{
  session_ = session;
});
server.on('stream', common.mustCall((stream) => {
  let sum = 0;
  stream.pause();
  const slowRead = ()=>{
    setTimeout(()=>{
      const data = stream.read(stream._readableState.highWaterMark/10);
      sum += data ? data.length: 0;
      console.log('read:' + sum + ' soc:' + socket.bytesWritten + ' closed:' + stream.closed + ' destroyed:' + stream.destroyed);
      if(stream.closed){ // Got RST_STREAM and stream was closed but all data isn't processed.
        socket.destroy(); // destroy connection without goaway frame.
        try{
          session_.ping(()=>{}); // activate read.
        }catch(err){
          console.log(err);
        }
      }
      slowRead();
    }, 10)
  };
  slowRead();
  stream.respond();
  stream.end();
  stream.on('error', (err)=>{
    // Stream allready closed, but error event happens.
    console.log(err);
  });
}));

let socket;
server.listen(0, common.mustCall(() => {
  const options = {
    createConnection: (authority, options) => {
      socket = net.connect(server.address().port, 'localhost');
      return socket;
    }
  }
  const client = http2.connect(`http://localhost:${server.address().port}`, options);
  const req = client.request({ ':method': 'POST' });

  req.on('response', common.mustCall());
  req.resume();
  const str = fs.createReadStream(loc);
  str.on('end', common.mustCall());
  str.pipe(req);
}));

[ronag]

From a purely streams perspective it's fine to emit 'error' as long as 'close' has not yet been emitted.

[sogaani]

@ronag I agree with it.
I just concerned a specific case.
Processing stream could fail by connection related error after a state of the stream closed, which means stream has received RST_STREAM and connection related error no longer affects a stream.

Maybe the subject of this issue is misleading.

[kanongil]

So the basic issue is that destroy(err) is unconditionally called on a stream that has been successfully closed by the remote side (in this case RST_STREAM with code 0)?

node/lib/internal/http2/core.js

Lines 1066 to 1071 in ff02801

	// Destroy any pending and open streams
	if (state.pendingStreams.size > 0 || state.streams.size > 0) {
	const cancel = new ERR_HTTP2_STREAM_CANCEL(error);
	state.pendingStreams.forEach((stream) => stream.destroy(cancel));
	state.streams.forEach((stream) => stream.destroy(error));
	}

[kanongil]

Or is it the deferred destroy() when reacting to the RST_STREAM that is buggy? Because, if it had already been destroyed, the destroy(err) from session destruction would never apply.

node/lib/internal/http2/core.js

Lines 514 to 534 in ff02801

	// Defer destroy we actually emit end.
	if (!stream.readable || code !== NGHTTP2_NO_ERROR) {
	// If errored or ended, we can destroy immediately.
	stream.destroy();
	} else {
	// Wait for end to destroy.
	stream.on('end', stream[kMaybeDestroy]);
	// Push a null so the stream can end whenever the client consumes
	// it completely.
	stream.push(null);
	// If the user hasn't tried to consume the stream (and this is a server
	// session) then just dump the incoming data so that the stream can
	// be destroyed.
	if (stream[kSession][kType] === NGHTTP2_SESSION_SERVER &&
	!stream[kState].didRead &&
	stream.readableFlowing === null)
	stream.resume();
	else
	stream.read(0);
	}

This seems to have been introduced in #18895.