http2: emit session close before stream close

Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #63414Fixes: #63412
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Author: mcollina

doc/api/http2.md

@@ -1113,10 +1113,12 @@ creates and returns an `Http2Stream` instance that can be used to send an
 HTTP/2 request to the connected server.
 
 When a `ClientHttp2Session` is first created, the socket may not yet be
-connected. if`clienthttp2session.request()` is called during this time, the
+connected. If`clienthttp2session.request()` is called during this time, the
 actual request will be deferred until the socket is ready to go.
-If the `session` is closed before the actual request be executed, an
-`ERR_HTTP2_GOAWAY_SESSION` is thrown.
+
+If the session becomes unavailable before the request can be created, the
+returned stream will emit `ERR_HTTP2_GOAWAY_SESSION` or
+`ERR_HTTP2_INVALID_SESSION` asynchronously.
 
 This method is only available if `http2session.type` is equal to
 `http2.constants.NGHTTP2_SESSION_CLIENT`.

lib/internal/http2/core.js

@@ -838,6 +838,10 @@ function requestOnConnect(headersList, options) {
 }
 }
 
+functionrequestOnError(error){
+this.destroy(error);
+}
+
 // Validates that priority options are correct, specifically:
 // 1. options.weight must be a number
 // 2. options.parent must be a positive number
@@ -1153,7 +1157,7 @@ function setupHandle(socket, type, options) {
 process.nextTick(emit,this,'connect',this,socket);
 }
 
-// Emits a close event followed by an error event if err is truthy. Used
+// Emits an error event followed by a close event if err is truthy. Used
 // by Http2Session.prototype.destroy()
 functionemitClose(self,error){
 if(error)
@@ -1224,17 +1228,16 @@ function closeSession(session, code, error) {
 session.setTimeout(0);
 session.removeAllListeners('timeout');
 
+constsocket=session[kSocket];
+consthandle=session[kHandle];
+
 // Destroy any pending and open streams
 if(state.pendingStreams.size>0||state.streams.size>0){
 constcancel=newERR_HTTP2_STREAM_CANCEL(error);
 state.pendingStreams.forEach((stream)=>stream.destroy(cancel));
 state.streams.forEach((stream)=>stream.destroy(error));
 }
 
-// Disassociate from the socket and server.
-constsocket=session[kSocket];
-consthandle=session[kHandle];
-
 // Destroy the handle if it exists at this point.
 if(handle!==undefined){
 handle.ondone=finishSessionClose.bind(null,session,error);
@@ -1809,11 +1812,15 @@ class ClientHttp2Session extends Http2Session {
 request(headersParam,options){
 debugSessionObj(this,'initiating request');
 
-if(this.destroyed)
-thrownewERR_HTTP2_INVALID_SESSION();
-
-if(this.closed)
-thrownewERR_HTTP2_GOAWAY_SESSION();
+// Keep argument validation synchronous, but defer session-state failures
+// to the returned stream so request retries from stream callbacks do not
+// throw before session lifecycle handlers run.
+letrequestError;
+if(this.destroyed){
+requestError=newERR_HTTP2_INVALID_SESSION();
+}elseif(this.closed){
+requestError=newERR_HTTP2_GOAWAY_SESSION();
+}
 
 this[kUpdateTimer]();
 
@@ -1899,19 +1906,24 @@ class ClientHttp2Session extends Http2Session {
 }
 }
 
-constonConnect=reqAsync.bind(requestOnConnect.bind(stream,headersList,options));
-if(this.connecting){
-if(this[kPendingRequestCalls]!==null){
-this[kPendingRequestCalls].push(onConnect);
+if(requestError){
+process.nextTick(reqAsync.bind(requestOnError.bind(stream,requestError)));
+}else{
+constonConnect=reqAsync.bind(
+requestOnConnect.bind(stream,headersList,options));
+if(this.connecting){
+if(this[kPendingRequestCalls]!==null){
+this[kPendingRequestCalls].push(onConnect);
+}else{
+this[kPendingRequestCalls]=[onConnect];
+this.once('connect',()=>{
+this[kPendingRequestCalls].forEach((f)=>f());
+this[kPendingRequestCalls]=null;
+});
+}
 }else{
-this[kPendingRequestCalls]=[onConnect];
-this.once('connect',()=>{
-this[kPendingRequestCalls].forEach((f)=>f());
-this[kPendingRequestCalls]=null;
-});
+onConnect();
 }
-}else{
-onConnect();
 }
 
 if(onClientStreamCreatedChannel.hasSubscribers){

test/parallel/test-http2-client-destroy.js

@@ -81,7 +81,19 @@ const { listenerCount } = require('events');
 assert.throws(()=>client.ping(),sessionError);
 assert.throws(()=>client.settings({}),sessionError);
 assert.throws(()=>client.goaway(),sessionError);
-assert.throws(()=>client.request(),sessionError);
+
+constpendingReq=client.request();
+pendingReq.on('response',common.mustNotCall());
+pendingReq.on('error',common.expectsError(sessionError));
+pendingReq.on('close',common.mustCall());
+
+client.on('close',common.mustCall(()=>{
+constpostCloseReq=client.request();
+postCloseReq.on('response',common.mustNotCall());
+postCloseReq.on('error',common.expectsError(sessionError));
+postCloseReq.on('close',common.mustCall());
+}));
+
 client.close();// Should be a non-op at this point
 
 // Wait for setImmediate call from destroy() to complete
@@ -92,7 +104,6 @@ const { listenerCount } = require('events');
 assert.throws(()=>client.ping(),sessionError);
 assert.throws(()=>client.settings({}),sessionError);
 assert.throws(()=>client.goaway(),sessionError);
-assert.throws(()=>client.request(),sessionError);
 client.close();// Should be a non-op at this point
 }));
 

test/parallel/test-http2-client-session-close-before-stream-close.js

@@ -0,0 +1,59 @@
+'use strict';
+
+constcommon=require('../common');
+if(!common.hasCrypto)
+common.skip('missing crypto');
+
+constassert=require('assert');
+consthttp2=require('http2');
+
+constserver=http2.createServer();
+letserverSocket;
+
+server.on('connection',common.mustCall((socket)=>{
+serverSocket=socket;
+socket.on('error',()=>{});
+}));
+
+server.on('sessionError',()=>{});
+server.on('stream',common.mustCall((stream,headers)=>{
+if(headers[':path']==='/close'){
+stream.respond({':status': 200});
+stream.write('partial',common.mustCall(()=>{
+setImmediate(()=>serverSocket.destroy());
+}));
+return;
+}
+
+stream.respond({':status': 200});
+stream.end('ok');
+}));
+
+server.listen(0,common.mustCall(()=>{
+constsession=http2.connect(`http://localhost:${server.address().port}`);
+letcachedSession=session;
+
+session.on('error',()=>{});
+session.on('close',common.mustCall(()=>{
+cachedSession=undefined;
+server.close();
+}));
+
+constreq=session.request({':path': '/close'});
+req.on('response',common.mustCall());
+req.on('error',()=>{});
+req.on('close',common.mustCall(()=>{
+// This must not throw synchronously even though the session is no longer
+// usable. Depending on teardown timing, the returned stream may report a
+// closed session before the destroy state is fully observable here.
+constreq2=session.request({':path': '/again'});
+
+req2.on('error',common.mustCall((err)=>{
+assert.ok(
+err.code==='ERR_HTTP2_INVALID_SESSION'||
+err.code==='ERR_HTTP2_GOAWAY_SESSION');
+assert.strictEqual(cachedSession,undefined);
+}));
+}));
+req.resume();
+}));