Skip to content
This repository

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

Closed
SaltwaterC opened this Issue · 5 comments

5 participants

Ștefan Rusu Tom Croucher Joseph Gentle Ben Noordhuis Mikeal Rogers
Ștefan Rusu

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.

Ștefan Rusu SaltwaterC referenced this issue from a commit
Ștefan Rusu SaltwaterC Test to prove #1399. 141a281
Ștefan Rusu SaltwaterC referenced this issue from a commit
Ștefan Rusu SaltwaterC Fixes #1399. 7b80c33
Tom Croucher

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

Joseph Gentle

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)
Joseph Gentle

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

Ben Noordhuis

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.

Ben Noordhuis bnoordhuis referenced this issue from a commit in bnoordhuis/node
Ben Noordhuis bnoordhuis http: emit Error object after .abort()
It was emitting the net.Socket object due to misuse of the arguments object.

Fixes #1399.
8acc0e4
Ben Noordhuis bnoordhuis closed this issue from a commit
Ben Noordhuis bnoordhuis http: emit Error object after .abort()
It was emitting the net.Socket object due to misuse of the arguments object.

Fixes #1399.
359a65a
Emerson Macedo emerleite referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
Mikeal Rogers

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
Something went wrong with that request. Please try again.