HTTP client request.abort() race condition for requests with bodies #1399

Closed
SaltwaterC opened this Issue Jul 26, 2011 · 5 comments

Projects

None yet

5 participants

@SaltwaterC

Here's a test that proves it:

var common = require('../common');
var assert = require('assert');
var http = require('http');

var server = http.createServer(function(req, res) {
  res.writeHead(200);
  res.end();
});

server.listen(common.PORT, function() {
  var cont = 'foo=bar';
  var opt = {
    port: common.PORT,
    method: 'POST',
    headers: {
      'content-type': 'application/x-www-form-urlencoded; charset=utf-8',
      'content-length': cont.length
    }
  };
  var req = http.request(opt);
  req.write(cont);
  req.abort();
  req.end(); // the race condition happens here as the connection is destroyed by req.abort()
  server.close();
});

Still trying to figure out if this is related to #796. Couldn't write a server that reproduces the "TypeError: Cannot read property '_httpMessage' of null" exception. This client does it every time.

@sh1mmer
sh1mmer commented Oct 26, 2011

This patch looks good to me. @ryah can you take a quick look?

@josephg
josephg commented Nov 1, 2011

I think I'm experiencing the same bug.

http = require 'http'

http.createServer (req, res) ->
  res.writeHead 200, {'Content-Type': 'text/plain'}
  # not calling res.end()
.listen 3000, ->
  req = http.get {host:'127.0.0.1', path:'/', port:3000}, (res) ->
    # This is never called - I wonder if the head is buffered waiting for data?
    console.log res.statusCode

  setTimeout (-> req.abort()), 1000

  # This error handler gets called.
  req.on 'error', (e) -> console.warn e.stack
$ coffee cc.coffee 
Error: socket hang up
    at createHangUpError (http.js:1083:15)
    at Socket.<anonymous> (http.js:1166:27)
    at Socket.emit (events.js:88:20)
    at Array.0 (net.js:280:10)
    at EventEmitter._tickCallback (node.js:194:26)
@josephg
josephg commented Nov 1, 2011

@sh1mmer I think you need to type @ry to bump him

@bnoordhuis
Member

I think this bug has been fixed in the new http library but @SaltWaterC's test demonstrates another issue. His test case, modified to listen for errors:

var common = require('../test/common');
var assert = require('assert');
var http = require('http');

var server = http.createServer(function(req, res) {
  res.writeHead(200);
  res.end();
});

server.listen(common.PORT, function() {
  var cont = 'foo=bar';
  var opt = {
    port: common.PORT,
    method: 'POST',
    headers: {
      'content-type': 'application/x-www-form-urlencoded; charset=utf-8',
      'content-length': cont.length
    }
  };
  var req = http.request(opt);
  req.on('error', function(e) {
    console.error(e);
    console.trace();
  });
  req.write(cont);
  req.abort();
  req.end(); // the race condition happens here as the connection is destroyed by req.abort()
  server.close();
});

Produces this:

{ _handle: null,
  _pendingWriteReqs: 0,
  _flags: 0,
  _connectQueueSize: 0,
  destroyed: true,
  bytesRead: 0,
  bytesWritten: 156,
<snip>
Trace
    at ClientRequest.<anonymous> (/home/bnoordhuis/src/nodejs/node/tmp/issue1399.js:23:13)
    at ClientRequest.emit (events.js:67:17)
    at Socket.<anonymous> (http.js:1115:11)
    at Socket.emit (events.js:67:17)
    at Array.0 (net.js:278:25)
    at EventEmitter._tickCallback (node.js:192:40)

Looks like the net.Socket object is emitted as an error event.

@bnoordhuis bnoordhuis added a commit to bnoordhuis/node that referenced this issue Nov 3, 2011
@bnoordhuis bnoordhuis http: emit Error object after .abort()
It was emitting the net.Socket object due to misuse of the arguments object.

Fixes #1399.
8acc0e4
@bnoordhuis bnoordhuis added a commit that closed this issue Nov 3, 2011
@bnoordhuis bnoordhuis http: emit Error object after .abort()
It was emitting the net.Socket object due to misuse of the arguments object.

Fixes #1399.
359a65a
@bnoordhuis bnoordhuis closed this in 359a65a Nov 3, 2011
@SaltwaterC SaltwaterC referenced this issue Nov 7, 2011
Closed

Fixes #1399 #1400

@mikeal
Member
mikeal commented Nov 10, 2011

just saw this fix, thanks! looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment