Node: Socket hang up on Node 8

[martinkuba]

• Version: 8.0.0
• Platform: OS X
• Subsystem: http

With the following

var http = require('http');

var server = http.createServer((req, res) => {
  console.log('got request');
  res.write('efgh')
  res.end();
});

server.listen(8080, function() {
  makeRequest(() => {
    server.close();
  });
});

function makeRequest(cb) {
  var req = http.request('http://localhost:8080', function(res) {
    console.log('got response')
    res.resume();
    cb();
  })
  req.end('abcd');
}

I get this error

events.js:182
      throw er; // Unhandled 'error' event
      ^

Error: socket hang up
    at createHangUpError (_http_client.js:343:15)
    at Socket.socketOnEnd (_http_client.js:435:23)
    at emitNone (events.js:110:20)
    at Socket.emit (events.js:207:7)
    at endReadableNT (_stream_readable.js:1045:12)
    at _combinedTickCallback (internal/process/next_tick.js:102:11)
    at process._tickCallback (internal/process/next_tick.js:161:9)

The same code works fine on Node 7 and 6. The key to reproducing seems to be that req.end() is called with data and res.write() is called with data before calling res.end().

[gireeshpunathil]

Simplified scenario:

var http = require('http')
http.createServer((req, res) => {
}).listen(25000, () => {
  http.request('http://localhost:25000', (res) => {
  }).end(' ')
})

It fails in more platforms and more versions.
if I use the same client and a remote web end-point for server, it works fine.
As @martinkuba pointed out, if end is called without data, no exception is seen.
Also if the server is responding atomically, no exception is seen.
Even if server delays responding, if it is atomic exception is seen:

http.createServer((req, res) => {
  setTimeout(() => res.end(), 1000);
})

[Michael-Antczak]

Version: 8.0.0
Platform: Ubuntu 16.04 LTS
Subsystem: http

Using @martinkuba server code I have the same error message on Ubuntu.

[diorahman]

This is because of wrong method being used? Since here https://github.com/nodejs/node/blob/master/lib/_http_client.js#L432 when the method is GET the req.res object is null.

If we change @martinkuba's line here:

var req = http.request('http://localhost:8080', function(res) {

to e.g. POST that makes useChunkedEncodingByDefault true.

var req = http.request({host: 'localhost', port: 8080, method: 'POST'}, function(res) {

It works: https://runkit.com/diorahman/post-13461

Not sure if we should throw a meaningful error while doing req.end(<PAYLOAD>) with the client request when the method is not right.

[gireeshpunathil]

ClientRequest.end specification does not impose any restrictions on the payload based on the method - so it looks like something else.

[refack fixed formating]

[diorahman]

@gireeshpunathil yeah. If you try to change this line https://github.com/nodejs/node/blob/master/lib/_http_client.js#L204 to true it will work as well.

Related discussion can be found here: https://github.com/nodejs/node/issues/3009 and here https://tools.ietf.org/html/rfc7231#section-4.3.1

[gireeshpunathil]

@diorahman - thanks for the info. I went though the history and came to an inference that malformed GET request can be rejected

However, I see inconsistencies in the client behavior based on the nature of client, nature of server and behavior of the server with respect to handling the parse error:

#cat q.js

'use strict'
const http = require('http')
const net = require('net')
const server = http.createServer((req, res) => {
  if (process.argv[2] === 'handle') {
    server.on('clientError', function(err) {
      console.log('parser error', err.code)
    })
  }
  setTimeout(() => {res.end('hello malformed client!'), 10})
})
server.listen(25000, () => {
  const malformdata = 'me too'
  if (process.argv[3] === 'http') {
    // HTTP CLIENT
    http.request({host: 'localhost', port: 25000, method:'GET', path: '/'}, (res) => {
      let data = ''
      res.on('data', function(d) {
        data += d
      })
      res.on('end', function() {
        console.log(data)
        server.close()
      })
    }).end(malformdata)
  } else {
    // TCP client
    const input = `GET / HTTP/1.1\r\nHost: localhost:25000\r\nConnection: close\r\n\r\n${malformdata}`
    const client = net.createConnection({host:'localhost', port: 25000}, () => {
      client.write(input)
    })
    client.on('error', (e) => {
      console.log(e)
    })
    client.on('data', (data) => {
      console.log('response: ' + data.toString())
      server.close()
    })
  }
})

#node q.js handle http
parser error HPE_INVALID_METHOD
hello malformed client!



md5-fa350e28dc83ffad6e7b9e918b7ce82c



#node q.js handle tcp
parser error HPE_INVALID_METHOD
response: HTTP/1.1 200 OK
Date: Fri, 09 Jun 2017 06:07:53 GMT
Connection: close
Content-Length: 23

hello malformed client!



md5-fa350e28dc83ffad6e7b9e918b7ce82c



#node q.js nohandle http
events.js:182
      throw er; // Unhandled 'error' event
      ^

Error: socket hang up
    at createHangUpError (_http_client.js:343:15)
    at Socket.socketOnEnd (_http_client.js:435:23)
    at emitNone (events.js:110:20)
    at Socket.emit (events.js:207:7)
    at endReadableNT (_stream_readable.js:1045:12)
    at _combinedTickCallback (internal/process/next_tick.js:102:11)
    at process._tickCallback (internal/process/next_tick.js:161:9)



md5-fa350e28dc83ffad6e7b9e918b7ce82c



#node q.js nohandle tcp
^C // HANG
#

Given that node can be acting as HTTP client in many of the distributed computing workloads, or delegating requests to remote end points, I guess the http / net client library need handle this cleanly.

I would like either:

• [ ] Server intercept the parse error, end response with closest HTTP error code mapping to parse error
• [x] Client intercepting malformed request, refuse to contact server, instead throw an error

@nodejs/http
@nodejs/streams
Also @refack @gibfahn

[gibfahn]

cc/ @nodejs/http @nodejs/streams

[mcollina]

I am a bit lost on what is the problem here. There have been different threads that points me to separate issues. Is this an http regression, or a stream regression?

Regarding the "socket hang up", I think that error is correct, the server is closing the connection abruptly, and then it's normal to have that error.

So maybe it was an HTTP change, and I think the current behavior is correct.

@gireeshpunathil can you add the links of the relative PRs?

[gireeshpunathil]

@mcollina - I don't have references to PRs, please treat my previous comment as an absolute problem statement.

I don't claim it a regression either, as I can reproduce this in earlier versions as well.

If socket 'hang up' is the right client behavior, we have these situations:

1. If server design is to close connection abruptly on malformed requests, then it has to be consistent. In this case,
   
   
   
   • It sends proper response, if we call res.end() from server as soon as it hits the request
   
   
   • It sends proper response, if we server has a client error handler.
   
   
   • In either case, it does not provide a meaningful message to the client, or the clien't caller.
   
   
2. If a TCP client is enployed, we get a 'hang' instead of a 'hang up'

(2) means that there is inconsistency at the client side as well.

Now that the clients are net and http, the entity which connect them is 'stream'. And hence I suspect this has bearing on http as well as streams.

Bottom line is that an error of type Unhandled 'error' event with Error: socket hang up does not qualify for and HTTP response on bad data, it could be proper for a low level network issue where the protocol layer dont have sufficient control.

We could very well reject this as noises through 'hacks' but then this behavior poses implications to web apps that forward remote requests that are not validated.

[mcollina]

I still do not understand the problem.

The http client does something more to handle the error condition, here is a fixed version of your script, it should produce similar behavior:

'use strict'
const http = require('http')
const net = require('net')
const server = http.createServer((req, res) => {
  if (process.argv[2] === 'handle') {
    server.on('clientError', function(err, sock) {
      console.log('parser error', err.code)
      console.log('destroying socket')
      sock.destroy() // the socket will not be destroyed automatically if you specify clientError
    })
  }
  setTimeout(() => {res.end('hello malformed client!'), 10})
})
server.listen(25000, () => {
  const malformdata = 'me too'
  if (process.argv[3] === 'http') {
    // HTTP CLIENT
    http.request({host: 'localhost', port: 25000, method:'GET', path: '/'}, (res) => {
      let data = ''
      res.on('data', function(d) {
        data += d
      })
      res.on('end', function() {
        console.log(data)
        server.close()
      })
    })
    .on('error', (err) => {
      console.log('error on request', err)
      server.close()
    })
    .end(malformdata)
  } else {
    // TCP client
    const input = `GET / HTTP/1.1\r\nHost: localhost:25000\r\nConnection: close\r\n\r\n${malformdata}`
    const client = net.createConnection({host:'localhost', port: 25000, allowHalfOpen: true })
    client.end(input)
    client.on('error', (e) => {
      console.log('tcp', e)
      server.close()
    })
    client.on('data', (data) => {
      console.log('response: ' + data.toString())
    })
    client.on('end', () => {
      console.log('client ended')
      server.close()
    })
  }
})

setTimeout(function () {
  console.log('normal termination')
}, 1000)

[mcollina]

We could very well reject this as noises through 'hacks' but then this behavior poses implications to web apps that forward remote requests that are not validated.

Does this mean you would like to accept requests that are not fully validated, in the example GET requests with a body?

[martinkuba]

I would like to point out that this was a regression from my point of view. My original repro works fine on previous versions of Node.

[gireeshpunathil]

@mcollina - thanks for this. Yes, this produces a consistent behavior.
So bad input is presented to the (node) server in the form of a 'clientError' event, and server is free to chose next action on the connection - I think that makes the server robust.

I will review the client side code and get back

[mcollina]

@martinkuba that HTTP request is invalid, as there should be no body for GET requests.
That causes the request to abort, and it will emit an 'error'  event.

Yes, it is a change of behavior from 6 and 7, but I think it is better in this way. The request is _invalid_, and the default behavior in the HTTP server is to destroy the socket without replying. If you want to handle it differently, you should specify a 'clientError' event handler.

cc @nodejs/http @jasnell @mscdex

[bnoordhuis]

Technically, HTTP allows GET requests with a body but it's useless in practice.

Because GET is idempotent, proxies and servers are not allowed to attach meaning to it; all a conforming recipient can do is ignore the body (ref).

Node 8's behavior is not wrong because it's not a reasonable thing to do in the first place. I'll close this out.

[mcollina]

To add some more context, in Node 6 and 7 the following has the same behavior of Node 8:

var http = require('http');

var server = http.createServer((req, res) => {
  console.log('got request');
  setTimeout(function () {
    res.write('efgh')
    res.end();
  }, 100)
});

server.listen(8082, function() {
  makeRequest(() => {
    server.close();
  });
});

function makeRequest(cb) {
  var req = http.request('http://localhost:8082', function(res) {
    console.log('got response')
    res.resume();
    cb();
  })
  req.end('abcd');
}

Node 8 made it consistent in all cases.

[Yugloocamai]

@gireeshpunathil confirmed

[amejiarosario]

@mcollina I have some clients with this issue. I don't have control over the client, so I'd like to provide a work on the server using 'clientError' event handler as you suggested. However, I'm struggling to get it working. How do I get the original request? I know I can use socket.end to reply but I don't see how to get the request...

      https.createServer(options, (req, res) => {
        // handle requests
      })
      .listen(port)
      .on('clientError', (err, socket) => {
        console.log('client error ->', err);
        // also handle requests somehow
        socket.end('HTTP/1.1 200 stuff\r\n\r\n');
      });

I'm also getting an error (I'm using HTTPS):

client error -> Error: 140737126245312:error:1407609C:SSL routines:SSL23_GET_CLIENT_HELLO:http request:../deps/openssl/openssl/ssl/s23_srvr.c:394:

    at Error (native)

[mcollina]

@amejiarosario what you get is not a request. You cannot get it. The only thing you can do in 'clientError'  is sending a specialized 5xx code, or customize the error.

[abusada]

for fellows still getting this error, i used longjohn which would print long stack traces, it helped me find the root cause of the error.