HTTP/2 — how to listen for connection close?

[yoshuawuyts]

• Version: v8.8.1 / v9.0.0
• Platform: Darwin
• Subsystem: HTTP/2

Heya! I'm trying to detect when a connection closes on an HTTP/2 connection in compat mode. We're trying to keep an SSE connection going, as long as the server and client are connected. In prior versions Node 8.x we could listen for it by attaching a listener to req.on('error') and req.connection.on('disconnect'):

    res.writeHead(200, {
      'Content-Type': 'text/event-stream',
      'Cache-Control': 'no-cache'
    })
    res.write('retry: 10000\n')

    var interval = setInterval(function () {
      res.write(`id:${id++}\ndata:{ "type:": "heartbeat" }\n\n`)
    }, 4000)

    // Attach an error handler, but no need to actually handle the error.
    // This is a bug in Node core according to mcollina which will be fixed
    // in a future Node release. Let's keep this in place as long as v8.x.x of
    // Node isn't in LTS yet.
    res.on('error', disconnect)
    req.on('error', disconnect)

    req.connection.addListener('close', disconnect, false)

    function disconnect () {
      clearInterval(interval)
      if (connected) {
        emitter.removeListener('scripts:bundle', reloadScript)
        emitter.removeListener('style:bundle', reloadStyle)
        connected = false
        state.sse -= 1
        if (!quiet) render()
      }
    }

source (choojs/bankai:http.js)

---

Since node v8.8.x (including node v9.0.x), this code no longer works. We're having trouble listening reliably for disconnects, which sometimes causes writes to happen after the connection has occurred:

A critical error occured, forcing Bankai to abort:

Error [ERR_HTTP2_STREAM_CLOSED]: The stream is already closed
    at Http2ServerResponse.write (internal/http2/compat.js:562:19)
    at Timeout._onTimeout (/Users/anon/src/choojs/bankai/http.js:192:11)
    at ontimeout (timers.js:471:11)
    at tryOnTimeout (timers.js:306:5)
    at Timer.listOnTimeout (timers.js:266:5)

If you think this might be a bug in Bankai, please consider helping
improve Bankai's stability by submitting an error to:

  https://github.com/choojs/bankai/issues/new

Please include the steps to reproduce this error, the stack trace
printed above, your version of Node, and your version of npm. Thanks!
— Team Choo

So I'm curious how to reliably listen for connection disconnects in the latest version of HTTP/2. You can try and reproduce this by cloning the latest bankai master, running npm install and npm start. Open up a browser window, and hit f5 a bunch — at some point a write after end should occur.

Hopefully this is enough info; let me know if I can help in any way. Thanks!

cc/ @jasnell @mcollina

Ref

• node v8.8 - HTTP2 issue?  choojs/bankai#313

[apapirovski]

@yoshuawuyts could you just do req.on('finish', disconnect)? That will always fire AFAIK... You don't need an 'error' handler either, that issue is fixed.

In general, I don't recommend using socket/connection for events or almost anything other than stuff like remoteAddress and potentially some of the TLS stuff. The reason the code above broke is that we're not longer attaching event listeners to the socket but rather the Http2Stream (which currently doesn't fire a close event — although it might again in the future once we figure out the semantics of it).

Also, strictly speaking, the code above wasn't working either since the stream could close while the session & connection remain open.

[apapirovski]

Hi @yoshuawuyts — any luck with the above? Let me know if it still doesn't work as expected :)

[yoshuawuyts]

@apapirovski thanks for getting back to me — testing out right now; I didn't get a chance to try it out yesterday.

[yoshuawuyts]

No, unfortunately it doesn't solve the issue 😞 — still getting the same error.

diff.patch

 diff --git a/http.js b/http.js
index a72b217..5499bcd 100644
--- a/http.js
+++ b/http.js
@@ -192,14 +192,7 @@ function start (entry, opts) {
       res.write(`id:${id++}\ndata:{ "type:": "heartbeat" }\n\n`)
     }, 4000)

-    // Attach an error handler, but no need to actually handle the error.
-    // This is a bug in Node core according to mcollina which will be fixed
-    // in a future Node release. Let's keep this in place as long as v8.x.x of
-    // Node isn't in LTS yet.
-    res.on('error', disconnect)
-    req.on('error', disconnect)
-
-    req.connection.addListener('close', disconnect, false)
+    req.on('finish', disconnect)

     function disconnect () {
       clearInterval(interval)

[apapirovski]

Do you have automated tests for this? Or simplest steps to replicate? Would like to try it. Surprised 'finish' doesn't work...

[apapirovski]

Ugh, I'm an idiot. Typo... it's res not req that the event should be listened for on. I'm reasonably certain that will work based on checking our source. All the ways that trigger a stream to close also trigger the res to emit finish. Sorry about that!

[yoshuawuyts]

@apapirovski all good, thanks for being on top of this! Though unfortunately that doesn't seem to work either :(

patch.diff

diff --git a/http.js b/http.js
index a72b217..0dbb304 100644
--- a/http.js
+++ b/http.js
@@ -192,14 +192,7 @@ function start (entry, opts) {
       res.write(`id:${id++}\ndata:{ "type:": "heartbeat" }\n\n`)
     }, 4000)
 
-    // Attach an error handler, but no need to actually handle the error.
-    // This is a bug in Node core according to mcollina which will be fixed
-    // in a future Node release. Let's keep this in place as long as v8.x.x of
-    // Node isn't in LTS yet.
-    res.on('error', disconnect)
-    req.on('error', disconnect)
-
-    req.connection.addListener('close', disconnect, false)
+    res.on('finish', disconnect)
 
     function disconnect () {
       clearInterval(interval)

[apapirovski]

Ok, that is surprising. Let me test locally.

[yoshuawuyts]

Also worth noting that refreshing a few times on the npm start output in Bankai seems to work fine - but if you smash f5 long enough you'll hit a case where it crashes.

I've got a few larger projects I'm testing this version in by symlinking it, and it tends to crash faster there.

[apapirovski]

Hmmm... I wonder if there's an issue elsewhere though because I setup the basic test case from the issue referenced above and I couldn't get it to fail. It failed reliably before the change to use res.on('finish', disconnect).

[yoshuawuyts]

@apapirovski I think a fair resolution at the moment would be to submit the patch you proposed for now. Thanks a lot!

Might be worth noting too that we haven't received any reports of similar issues with http/2 disabled. It might be worth staying on the lookout for more failures. But I'm probably stating the obvious here, as http/2 is still marked "experimental" ☺️. Thanks heaps!