HTTP/1 request destroy behavior change on framing error

[dougwilson]

• Version: v10.13.0
• Platform: Windows 10
• Subsystem: http

I noticed a different in how the HTTP/1 request objects are left after a HTTP/1 framing error (which causes an error in the underlying http_parser). In Node.js 8 and below, the request object would be destroyed and be left in non-readable state (i.e. req.readable === false). It seems like in Node.js 10+ this is no longer the case. Is this an expected change? Express.js has underlying machinery that is looking to see if a request body should be read (since the request starts out in a pasued state since 0.10) and it doesn't attempt to read when it's no longer readable (since events like 'end' will not be emitted, leaving things hanging around forever).

Here is an example that reproduces the scenario:

var http = require('http')
var net = require('net')

var server = http.createServer(function (req, res) {
  setTimeout(function () {
    if (!req.socket.readable) {
      console.log('req already terminated')
      server.close()
      return
    }

    console.log('req read start')

    var bufs = []

    req.on('data', function (chunk) {
      console.log('request recv %d bytes', chunk.length)
      bufs.push(chunk)
    })

    req.on('end', function () {
      var data = Buffer.concat(bufs)
      console.log('request got %d bytes', data.length)
      res.end('OK')
    })

    req.on('close', function () {
      console.log('clean up here...')
      server.close()
    })
  }, 5000)
})

server.listen(0, function () {
  var port = server.address().port
  var sock = net.createConnection(port, '127.0.0.1')

  sock.on('connect', function () {
    sock.write('POST / HTTP/1.1\r\n')
    sock.write('Host: localhost\r\n')
    sock.write('Transfer-Encoding: chunked\r\n')
    sock.write('\r\n')
    sock.write('W')
    sock.on('close', function () {
      console.log('client socket closed')
    })
  })
})

setTimeout(function () {
  console.log('timeout!')
  process.exit(1)
}, 10000).unref()

In Node.js 8 and lower you get the following output:

client socket closed
req already terminated

In Node.js 10 you get the following output:

req read start
timeout!

I'm not sure if this was an expected change or not. If it is an expected change, I'm just looking for what the correct way to know if a bit of code (the part inside setTimeout) that runs at an arbitrary time after the request came in should know if it's actually ever going to get a complete read or not. The only thing I've found so far in Node.js 10 is that you have to try and read no matter what and just rely on coding in a read timeout to know when to give up instead of knowing earlier.

[addaleax]

@nodejs/http

[addaleax]

Bisecting points to 9b7a691.

/cc @lpinca

[lpinca]

I think the problem is that the client socket is paused. If you add sock.resume() after the 'connect' event is emitted, it works as expected.

[dougwilson]

Thanks @lpinca . So there are I guess two issues with that suggestion: (1) this was just a replication all within Node.js, but originally the client was a C library, not Node.js, and (2) the question here is really about the server; I don't think the server really gets a say over what the clients are doing in the end (I mean, the client sent bad framing already :) ).

Is the suggested solution here that whatever client code out there just needs to be altered such that Node.js streams on the server code are acting like they used to? Or are you saying something else?

[dougwilson]

Just thinking more about how to articulate my thoughts: I feel like when a framing error occurs when the client is transmitting the body of the http request, there should be some way to understand on the req that this happened and the request is in an error state or some such. There really just seems like no way to do this in Node.js 10 as far as I can tell (I opened a related issue #24585 about that). Of course in the other issue, it was an event on socket that emitted even prior to attaching a 'data' event listener when the request stream starts out in paused mode, so I was using req.readable in prior versions to infer that an error occurred.

I'm just generally at a loss of how, in Node.js 10+, you can tell that a request is in an error state any more to do able to process it differently (for example, skip even bothering to read the request body, typically just req.resume() it to dump it out without storing it).

A typical framing mistake clients will make in chunked mode is they end the request with \r\n\r\n improperly, instead of with a zero-length chunk (\r\n0\r\n\r\n).

It seems that one difference here is that in Node.js 8 and below this mistake would end up calling .destroy() on the object, which set the request object to be in the non-readable state, but now in Node.js 10+ there is some code that is instead writing a HTTP/1.1 400 Bad Request back and then I guess trying to gracefully close down the connection, which it seems that clients can then trigger this state if they are not reading off the socket on their side for whatever reason (clients with framing errors are just generally malfunctional, though).

[lpinca]

Is the suggested solution here that whatever client code out there just needs to be altered such that Node.js streams on the server code are acting like they used to?

No, I don't think so, the problem in the example is that no one is closing the TCP connection because there is buffered data to consume on the client. Only when buffered data is consumed on the client the socket is closed but this is how it works in Node.js. If the C lib was closing the socket then no changes should be necessary.

The question is more about how the parser handle errors. If the socket is not explicitly destroyed on error, it is still readable as long as there is buffered data to read.

[dougwilson]

the problem in the example is that no one is closing the TCP connection because there is buffered data to consume on the client. Only when buffered data is consumed on the client the socket is closed but this is how it works in Node.js. If the C lib was closing the socket then no changes should be necessary.

Right, but that is what I'm saying: the c client is not consuming the data, just like the code I provided above. It was working in node.js 8. How can I get the server to behave the same way in Node.js 10 given the same client behavior?

[lpinca]

Make the client close the connection? One of the two peer must do that. If that particular client (not affected by the Node.js breaking change) was doing it before, why isn't it doing now? Or does it?

[dougwilson]

And to clarify: what I mean by not consuming is just the term you were using. Specifically in the c code, it is reading the data, but it does not send back a ACK to the server's FIN, which is why Node.js thinks the other side is open still (I used Wireshark to see the exact packets being exchanged). The c client sees the HTTP 400 Bad Request response, but it's some poorly written thing by a vendor and since that doesn't have connection: close it it assuming the default http/1.1 behavior of keep alive I guess? I don't know why it does not ACK the server's FIN.

But what I can probably do in Express.js is just also check the writable state of the socket. Probably just assume when socket.writable === false, then don't bother trying to read the request since something has happened where the socket is half closed now. I don't exactly understand yet what the ramifications of such an assumption would be, though. Ideally if there was a way to know that there was a parser error that got the node.js side into this state...

[dougwilson]

Make the client close the connection? One of the two peer must do that. If that particular client (not affected by the Node.js breaking change) was doing it before, why isn't it doing now? Or does it?

It is some vendor code, I can't just alter the client code... I just want to be able to skip reading a request that is in a broken state on the node.js side is all. Lots of clients are going to interact with a server that are not under the control of the same people who control the server, right?

[lpinca]

Yeah definitely. I think one way is to use the 'clientError' event which is emitted on parse error and destroy the socket, like it was done before f2f391e.

[dougwilson]

But that doesn't really help in my example above. The example is that there is code that executes to read a request (the body of setTimeout) that runs at a later point (after doing things like looking up and validating auth headers).

Can you share what an example would look like to out un place of the req.socket.readable in your example?

Or are you saying that the solution is that it will just work as it as long as I add a custom clientError event listener to the server object that just reinstates the old socket.destroy() behavior? That's unfortunately not possible to make a change in Express.js to fix the behavior for users since Express.js does not create the server object to attach event listeners to; it just returns the requestListener function the user needs to pass to their own http (or https) server.

[lpinca]

Or are you saying that the solution is that it will just work as it as long as I add a custom clientError event listener to the server object that just reinstates the old socket.destroy() behavior?

This.

That's unfortunately not possible to make a change in Express.js to fix the behavior for users since Express.js does not create the server object to attach event listeners to; it just returns the requestListener function the user needs to pass to their own http (or https) server.

How about this hack:

function destroy(err, socket) {
  socket.destroy(err)
}

and inside the 'request' listener add

req.socket.server.on('clientError', destroy);

Edit: ofc the event listener should be added only once.

[dougwilson]

Oh, didn't realize there was a server property on the socket. I don't see that documented anywhere, is that public API? So two questions on that, though (1) would there be any negative consequences doing that if existing servers are doing something in that event and (2) what is a good way to only add the event listener once?