Parse Error in http.js #1956

Closed
japj opened this Issue Oct 28, 2011 · 27 comments

Projects

None yet

10 participants

@japj

Hi,

I am using nodejs 0.5.10 on Windows, using nano 0.8.4 to connect to a couchdb (1.0.2) database.

This night my program failed with the following notice:

node.js:202
        throw e; // process.nextTick error, or 'error' event on first tick
              ^
Error: Parse Error
    at Socket.ondata (http.js:1116:24)
    at TCP.onread (net.js:314:27)

It is a bit difficult to relate, but around the same time the nodejs program threw an exception I see a '500' error in the couchdb logging. Maybe that is related?

@bnoordhuis
Node.js Foundation member

Can you trace the request with tcpdump or wireshark?

@japj

I have not been able to reproduce it yet, but the couchdb server is running on centos 6 so if it is enough to set this up on that machine (provided I have enough instructions) then I can set that up and 'hope' it occurs again.

@bnoordhuis
Node.js Foundation member

I suspect there is no real Node bug here but that instead it's some error condition on the server side, seeing that couchdb logs a 500.

If couchdb prematurely closes the connection, an error will bubble up on the client side that you should be able to catch if your request object has an error event listener.

But if you want to know exactly what is going on, you'll have to trace the traffic going over the wire.

@japj

The only 'strange' thing I saw was that http.js:1116:24 relates to:

var ret = parser.execute(d, start, end - start);

at https://github.com/joyent/node/blob/master/lib/http.js#L1136

and in https://github.com/joyent/node/blob/master/src/node_http_parser.cc#L466 it generates the exception object which it then returns (as far as I can tell)?.

How should this Parse Error exactly propogate from node_http_parser.cc to http.js and then to the client side?

@japj

btw, I'm using the nano couchdb library and from looking at the nano sources it seems errors should propogate from it to my client code, so I am not entirely sure why I didnt get an error in my client instead of the above exception.

@bnoordhuis
Node.js Foundation member

How should this Parse Error exactly propogate from node_http_parser.cc to http.js and then to the client side?

socket.destroy(exception) is called on parse errors. That exception is emitted as an error event on the socket that the error listener in http.js in turn emits as an error event on the request object.

@mikeal
Node.js Foundation member

yeah, this error should emit on the http.ClientRequest object if you're using 0.5.4+, if not it's a bug.

@windyrobin

@bnoordhuis
@mikeal

After some hard work, I finally could reproduce it
node v0.5.10

server.js

var http = require("http");

 var server = http.createServer(function(req, res){
   res.writeHead({"Content-Type" : "text/html"});
   res.end("hello,world\n");
});

server.listen(3458);

get.js

 var http = require("http");

 var options = {
   port: 3458,
   path: '/'
 };

 http.get(options, function(res) {
   console.log("Got response: " + res.statusCode);
 }).on('error', function(e) {
   console.log("Got error: " + e.message);
 });

then I execute some cmds :

$ node server.js &
[1] 13034
$ curl localhost:3458/
hello,world
$ node get.js 
Got error: Parse Error

Note that when I write the server and the client in the same file ,
it runs ok (this's why the test/test-http* couldn't detect the error)

parse.js

var http = require("http");

 var server = http.createServer(function(req, res){
   res.writeHead(200, {"Content-Type" : "text/html"});
   res.end("hello,world\n");
 });

 const PORT = 3458;
 var options = {
   port : PORT,
   path :'/'
 }

 server.listen(PORT , function(){
   http.get(options, function(){
     console.log("get a response");
   }).on("error", function(err){
     console.log(err.message);
   })
 })

the result

$node parse.js 
get a response

and I also found another issue which created by me
is caused by this serious bug!!!
#1958

@siddMahen

On top of this, if you don't console.log() the error in get.js, node runs without crashing, even though it should crash on an unhandled error.

@bnoordhuis
Node.js Foundation member

@windyrobin: You forget to pass the status code in the res.writeHead() call in server.js.

@siddMahen: There is no unhandled error here, your error listener is simply a no-op.

@japj: Can I close this issue?

@windyrobin

@bnoordhuis

I'm so sorry , I'am dizzy on this problem...

Besides that ,could you please give some tips on how to make http-keep-alive requests,
I don't want want the agent.socket be destroied when there is no pending request ,then I could
reuse the socket again in some mill-seconds later...

@bnoordhuis
Node.js Foundation member

@windyrobin You are mixing issues, stick to #1958.

@japj

@bnoordhuis I still don't understand why I am getting the error with this specific stracktrace.

As far as I can tell, nano uses mikeal's request library.

The request library will call the nano callback whenever an error occurs during http requests at https://github.com/mikeal/request/blob/master/main.js#L163

Nano will call my callback and will pass the error through at https://github.com/dscape/nano/blob/master/nano.js#L144

So as far as I can tell, everyone is properly trying to handle the error right?
Then why does it go wrong?

@bnoordhuis
Node.js Foundation member

You know that as a rule we don't support third-party modules, right? Can you reproduce the problem with a plain Node test case?

@japj

@bnoordhuis the http_parser code surrounding error handling has changed a fair bit (comparing 0.4.12 and 0.5.10). I am still trying to figure out if that could be the cause of the problem.

What would help in future analysis of similar issues is if nodejs http_parser.cc would actually use the error information from the http_parser in the exception (i.e. use http_errno_name/http_errno_description to fill the exception information it returns when generating a "Parse Errror"). I expect that is on the todo list though?

@bnoordhuis
Node.js Foundation member

What would help in future analysis of similar issues is if nodejs http_parser.cc would actually use the error information from the http_parser in the exception (i.e. use http_errno_name/http_errno_description to fill the exception information it returns when generating a "Parse Errror").

Sounds like a good idea.

I expect that is on the todo list though?

It is now. :-)

@SaltwaterC

Have something like this in node v0.4.12:

node.js:134
        throw e; // process.nextTick error, or 'error' event on first tick
        ^
Error: Parse Error
    at Socket.ondata (http.js:1231:22)
    at Socket._onReadable (net.js:677:27)
    at IOWatcher.onReadable [as callback] (net.js:177:10)

Still have to dig in order to find the problematic URL, but it does look like a node bug. There is an error listener for the request, therefore is not PEBCAK.

@SaltwaterC

Found the URL: http://www.dennisbabkin.com/php/download.php?go=ScrEncCtl&file=screncctrl.zip

It appears to be some sort of race condition, but I can't reproduce it with code handling this URL alone. With this URL alone, the error is properly passed to the error listener. I guess I should dig some more.

@dazagrohovaz

i have the same problem with the native parser
some ideas??

node version 0.6.0

SERVER ANWSER:
HTTP/1.0 200 Connection established\r\n\r\n

CODE:
socket.ondata = function(data, start, end) {
var ret = parser.execute(data, start, end - start); <<< here breaks...
if (ret instanceof Error) {
socket.destroy(ret);
}
}

ERROR:
node.js:201
throw e; // process.nextTick error, or 'error' event on first tick
^
Error: Parse Error
at Socket.ondata (/root/nodechain/lib/nodechain.js:293:22)
at TCP.onread (net.js:334:27)

@bnoordhuis
Node.js Foundation member

@dazagrohovaz: You're on your own here, the HTTP parser API is not for public consumption. That said, I added a 'no response headers' test in 8f15582 and it passes.

@jamespeerless

SaltwaterC - I am seeing the same issue as you. With that URL alone the error gets handled fine. If I am processing a lot of urls I'll eventually hit one where this error is not captured. I cannot figure it out, been poking around for hours :-/

@jamespeerless

In my case it looks like a bug with mikeal's request module. It looks like it's a race condition. It's a race between the error handler getting setup and the response returning (or emitting an error). Not entirely sure I am right but that's what it seems like. I'll update my issue on mikeal's request github.

Alternatively, what if the request emits an error before the ClientRequest object is returned so you could never have time to setup the error handler?

@mikeal
Node.js Foundation member

@bnoordhuis the parser is for public consumption as far as I know.

node-pcap uses it and I have a private project that also uses the parser directly. the API is very "close to the metal" and errors aren't expected to be as pretty as errors from http.js but AFAIK the http parser is a public API.

@bnoordhuis
Node.js Foundation member

@mikeal: No, the HTTP parser API is private. It can and has changed and likely will again. I don't mind if you use it but don't expect it to be stable or even to stay around.

@koichik

Related to #2206?
When the response body is longer than Content-Length, the Parse Error occurs.
And if Keep-Alive is used, the error does not propagate to http.ClientRequest because the response is already completed (The error occurs after the response).

reproduce:

var http = require('http');
var data = '¥'; // U+00A5 (2 bytes [0xC2, 0xA5] in UTF-8)
http.createServer(function(req, res) { 
  res.writeHead(200, {
    'Content-Type': 'text/plain',
    'Content-Length': data.length // ERROR!! should use Buffer.byteLength()
  });
  res.end(data); // writes 2 bytes
}).listen(3000, function() {
  http.get({port: 3000}, function(res) { 
    console.log(res.headers);
    res.on('data', function(data) {
      console.log('DATA', data);
    }).on('end', function() {
      console.log('END');
    });
  });
});

result:

{ 'content-type': 'text/plain',
  'content-length': '1',
  connection: 'keep-alive' }
DATA <Buffer c2>
END

node.js:201
        throw e; // process.nextTick error, or 'error' event on first tick
              ^
Error: Parse Error
    at Socket.ondata (http.js:1125:24)
    at TCP.onread (net.js:334:27)
@kilianc

Is it fixed? Having the same issue with large scraping tasks.
I am using @mikeal request module too.

@bnoordhuis
Node.js Foundation member

@kilianc: The bug that @koichik's test demonstrates is fixed in master but not v0.6.

@bnoordhuis bnoordhuis closed this Apr 1, 2012
@sam-github sam-github pushed a commit to sam-github/node-v0.x-archive that referenced this issue Jul 18, 2015
@rvagg rvagg 2015-06-23 io.js v2.3.1 Release
PR-URL: nodejs/node#1996

Notable changes

* module: The number of syscalls made during a require() have been
  significantly reduced again (see #1801 from v2.2.0 for previous
  work), which should lead to a performance improvement
  (Pierre Inglebert) #1920.
* npm:
  - Upgrade to v2.11.2 (Rebecca Turner) #1956.
  - Upgrade to v2.11.3 (Forrest L Norvell) #2018.
* zlib: A bug was discovered where the process would abort if the
  final part of a zlib decompression results in a buffer that would
  exceed the maximum length of 0x3fffffff bytes (~1GiB). This was
  likely to only occur during buffered decompression (rather than
  streaming). This is now fixed and will instead result in a thrown
  RangeError (Michaël Zasso) #1811.
8e53fd5
@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this issue Sep 18, 2015
@rvagg rvagg 2015-06-19 io.js v2.3.1 Release
PR-URL: nodejs/node#1996

Notable changes

* module: The number of syscalls made during a require() have been
  significantly reduced again (see #1801 from v2.2.0 for previous
  work), which should lead to a performance improvement
  (Pierre Inglebert) #1920.
* npm:
  - Upgrade to v2.11.2 (Rebecca Turner) #1956.
  - Upgrade to v2.11.3 (Forrest L Norvell) #2018.
* zlib: A bug was discovered where the process would abort if the
  final part of a zlib decompression results in a buffer that would
  exceed the maximum length of 0x3fffffff bytes (~1GiB). This was
  likely to only occur during buffered decompression (rather than
  streaming). This is now fixed and will instead result in a thrown
  RangeError (Michaël Zasso) #1811.
12f8bba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment