Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

HTTP POST / PUT race condition when using it with Expect: 100-continue #2636

Closed
SaltwaterC opened this issue Jan 29, 2012 · 6 comments
Closed
Labels

Comments

@SaltwaterC
Copy link

I managed to create a script that reproduces this. However, I didn't manage to create a self contained one (maybe due to laziness), but here it goes:

var http = require('http')
var qs = require('querystring')
var reqBody = qs.stringify({foo: 'bar'})
var opt = {
        method: 'POST',
        host: '127.0.0.1',
        path: '/index.php',
        headers: {
                expect: '100-continue',
                'content-length': reqBody.length,
                'content-type': 'application/x-www-form-urlencoded'
        }
}
var req = http.request(opt, function (res) {
        res.on('data', function (data) {
                console.log(data.toString())
        })
        res.on('end', function () {
                console.log('end')
        })
})
req.on('error', function (err) {
        console.error(err)
})
req.write(reqBody)
req.end()

The remote script is a simple:

<?php
var_dump($_REQUEST);

Here's what's logged into the shell:

$ node php.js 

http.js:1147
      var ret = parser.execute(d, start, end - start);
                       ^
TypeError: Cannot call method 'execute' of null
    at Socket.ondata (http.js:1147:24)
    at TCP.onread (net.js:354:27)
$ node php.js 

http.js:1147
      var ret = parser.execute(d, start, end - start);
                       ^
TypeError: Cannot call method 'execute' of null
    at Socket.ondata (http.js:1147:24)
    at TCP.onread (net.js:354:27)
$ node php.js 
array(1) {
  ["foo"]=>
  string(3) "bar"
}

end
$ node -v
v0.6.9

Couple of failures and a success. For the same script. For a larger HTTP client implementation, it fails all the time. Here's the Wireshark capture: https://gist.github.com/1700091

Without Expect: 100-continue everything is peachy.

@bnoordhuis
Copy link
Member

Confirmed.

@bnoordhuis
Copy link
Member

@koichik: Can you take a look at this? It seems to be a regression that was introduced in 35fe3eb.

@jnak
Copy link

jnak commented Jan 30, 2012

+1

@koichik
Copy link

koichik commented Jan 30, 2012

@SaltwaterC - Thanks for the report.

@bnoordhuis - Can you review koichik/node@f36b0af?

@bnoordhuis
Copy link
Member

@koichik: LGTM

@koichik
Copy link

koichik commented Jan 30, 2012

@bnoordhuis - Thanks, merged in 3fd13c6.

isaacs added a commit that referenced this issue Feb 1, 2012
* Update V8 to 3.8.9

* Support for sharing streams across Isolates (Igor Zinkovsky)

* #2636 - Fix case where http_parsers are freed too early (koichik)

* url: Support for IPv6 addresses in URLs (Łukasz Walukiewicz)

* child_process: Add disconnect() method to child processes (Andreas Madsen)

* fs: add O_EXCL support, exclusive open file (Ben Noordhuis)

* fs: more specific error messages (Tj Holowaychuk)

* tty: emit 'unknown' key event if key sequence not found (Dan VerWeire, Nathan Rajlich)

* build: compile release build too if BUILDTYPE=Debug (Ben Noordhuis)

* module: fix --debug-brk on symlinked scripts (Fedor Indutny)

* zlib: fix `Failed to set dictionary` issue (Fedor Indutny)

* waf: predict target arch for OS X (Fedor Indutny)
richardlau pushed a commit to ibmruntimes/node that referenced this issue Sep 18, 2015
Note: When this was cherry-picked for v8 v4.4 (landed in nodejs#2636),
test-heap.cc:1989 chunk did not exist. It now exists in
v8 v4.5.103.30. This PR completes the cherry pick of the whole commit
from v8.

PR-URL: nodejs/node#2692
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

Original commit message:
  Use static_cast<> for NULL (clang 3.7)

  The following errors come up when compiling v8
   with clang 3.7 on FreeBSD/amd64:

  src/runtime/runtime-i18n.cc:629:37: error: reinterpret_cast from
  'nullptr_t' to 'v8::internal::Smi *' is not allowed
    local_object->SetInternalField(1, reinterpret_cast<Smi*>(NULL));
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

  test/cctest/test-heap.cc:131:20: error: reinterpret_cast from
        'nullptr_t' to 'v8::internal::Object *' is not allowed
    Handle<Object> n(reinterpret_cast<Object*>(NULL), isolate);
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  test/cctest/test-heap.cc:1989:18: error: reinterpret_cast from
        'nullptr_t' to 'Address' (aka 'unsigned char *') is not
        allowed
    Address base = reinterpret_cast<Address>(NULL);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  +add myself to the AUTHORS file.

  BUG=

  Review URL: https://codereview.chromium.org/1277353002

  Cr-Commit-Position: refs/heads/master@{#30103}
richardlau pushed a commit to ibmruntimes/node that referenced this issue Sep 18, 2015
Note: When this was cherry-picked for v8 v4.4 (landed in nodejs#2636),
test-heap.cc:1989 chunk did not exist. It now exists in
v8 v4.5.103.30. This PR completes the cherry pick of the whole commit
from v8.

PR-URL: nodejs/node#2692
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

Original commit message:
  Use static_cast<> for NULL (clang 3.7)

  The following errors come up when compiling v8
   with clang 3.7 on FreeBSD/amd64:

  src/runtime/runtime-i18n.cc:629:37: error: reinterpret_cast from
  'nullptr_t' to 'v8::internal::Smi *' is not allowed
    local_object->SetInternalField(1, reinterpret_cast<Smi*>(NULL));
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

  test/cctest/test-heap.cc:131:20: error: reinterpret_cast from
        'nullptr_t' to 'v8::internal::Object *' is not allowed
    Handle<Object> n(reinterpret_cast<Object*>(NULL), isolate);
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  test/cctest/test-heap.cc:1989:18: error: reinterpret_cast from
        'nullptr_t' to 'Address' (aka 'unsigned char *') is not
        allowed
    Address base = reinterpret_cast<Address>(NULL);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  +add myself to the AUTHORS file.

  BUG=

  Review URL: https://codereview.chromium.org/1277353002

  Cr-Commit-Position: refs/heads/master@{#30103}
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants