Skip to content
This repository

The secure http response never fire the end event. #728

Closed
JacksonTian opened this Issue February 25, 2011 · 26 comments
Jackson Tian
       var https = require("https");
        //https://www.googleapis.com/urlshortener/v1/url
        var options = {
            host: 'www.googleapis.com',
            port: 443,
            path: '/urlshortener/v1/url',
            method: 'POST',
            headers: {
                'Content-Type': 'application/json'
            }
        };

        var request = https.request(options, function(response) {
            var data = '';

            response.on('data',function(chunk){
                data += chunk;
                process.stdout.write(chunk);
                // This print 
                //{
                // "kind": "urlshortener#url",
                // "id": "http://goo.gl/fbsS",
                // "longUrl": "http://www.google.com/"
                //}
            }).on('end', function (){
                console.log(data);
            });
        });

        request.write(JSON.stringify({"longUrl": "http://www.google.com"}));
        request.end();
Jackson Tian

The nodejs version is 0.5.0pre.

Paddy Foran

I can verify this is still an issue, and would love to see a fix for it.

Jonathan Rudenberg

This seems to be a regression introduced in v0.4.8:

var https = require('https');

https.get({ host: 'www.google.com', path: '/accounts/o8/site-xrds?hd=jadedpixel.com' }, function(res) {
  res.on('data', function(d) {
    console.log('chunk');
  });

  res.on('end', function() {
    console.log('end');
  });
}).on('error', function(e) {
  console.error(e);
});
$ /usr/local/Cellar/node/0.4.7/bin/node get.js
chunk
chunk
chunk
end
$ /usr/local/Cellar/node/0.4.8/bin/node get.js
chunk
chunk
chunk
$

See also #671.

bolinfest

This change seems like it may be the cause: 7c6f014

ry
ry commented May 27, 2011

The change is a result of 75a0cf9

joecroninallen

I assume this still hasn't been fixed because I encountered the bug just today with the latest v.0.4.8. Does anyone have any updates on this one? I can't use 0.4.8 until its fixed. It looks like this has been an open issue for a while.

bolinfest

I see that 0.4.9 and 0.5.0 (unstable) have been released, but this regression is still not fixed. It would be great if fixing regressions were higher priority -- I need to stick with 0.4.7 until this is fixed.

Jonathan Rudenberg

I agree that it's annoying that it hasn't been fixed, for now I'm using this backward/forward-compatible workaround:

function done() {
  // do stuff
}
res.on('end', done);
res.on('close', done);
Aaron Heckmann

looks like still an open issue in 0.4.9

Lior Cohen

Yup, still in 0.4.9

Dave Hoff

Confirmed, this issue is present in v0.5.0

nponeccop

The issue seems to be specific to certain servers. In 0.4.9 it is fired with many servers, but Google is a prominent exception.

Scott Brown

I am also experiencing this issue with Node.js 0.4.10 interacting with the Google OAuth2 API.

Jason Bond Pratt

Seeing this occasionally with Facebook/Node v0.4.9

jeffv

Seeing this occasionally node v0.4.10.
Doing 2500 https calls to amazon sqs and <10 of them close instead of end randomly.

ry
ry commented August 04, 2011

This isn't a bug. Responses are not guaranteed to have an 'end' event. Use 'close'.

jeffv

How do you mean?
Unless something went wrong with the connection I'm expecting an end event. If the server doesn't send FIN shouldn't it timeout from inactivity rather then closing without warning?

bolinfest

Also, in reading http://nodejs.org/docs/v0.4.10/api/streams.html#event_close_:

"Emitted when the underlying file descriptor has been closed. Not all streams will emit this."

As compared to http://nodejs.org/docs/v0.4.10/api/streams.html#event_end_:

"Emitted when the stream has received an EOF (FIN in TCP terminology). Indicates that no more 'data' events will happen."

'end' sounds like the more reliable event.

The behavior is different in 0.4.7, i.e., it did what developers expected. Are you saying that was not the correct behavior?

Mikeal Rogers

does close get emitted when the connection is keep-alive? cause i thought that close scoped to the underlying file descriptor.

jeffv

Also... as bolinfest stated you are not guaranteed to have a 'close', so you can't use it reliably either.
If you get 'end' you don't get 'close'. If you get 'close' you don't get 'end'.

Just like titanous suggested as a temporary fix you have to listen for both and double up on your callback, which is really clunky.

Liam R. Black

Should this be fixed or left as is? I noticed @ry said it wasn't a bug, but @jeffv said having both "end" and "done" were a temporary fix.

Also the issue hasn't been closed as won't fixed.

ry
ry commented October 06, 2011

i take back what i said before. we should probably make this work. cc @isaacs and @mikeal - this has a lot to do with stream interface.

Liam R. Black

Should one of the events be deprecated ( if so, which one) ?

I haven't looked enough at the code to understand how to properly fix the underlying events, but here is a hackish temporary fix for https.js

exports.request = function(options, cb) {
  if (options.agent === undefined) {
    options.agent = globalAgent;
  }
  options.createConnection = createConnection;
  options.defaultPort = options.defaultPort || 443;

  var request = http.request(options, cb);
 var on = request.on; 
 request.on = function(event,listener){
    if(event === 'end'){
      on.on('close',listener);
    }else if(event === 'close'){
      on.on('end',listener);
    }
    return on.on(event,listener);
  };
  return request;
};
Isaac Z. Schlueter
Collaborator

@limeblack

Should one of the events be deprecated ( if so, which one) ?

Yes, we need to organize this api a lot.

In my opinion, the "end" event should be guaranteed, and "close" should come afterwards as an optional "bindings are cleaned up" event.

jeffv

@isaacs

Having had a bit of time to think on this one that's exactly what I'd hoped someone would suggest. It would be really nice if one of the events fired no matter what at the end so that if you don't really care how something terminated you can still depend on it.

If I could always bind to 'end' and not have to worry about what circumstances 'close' happens under I'd be a very happy camper. Right now I have to bind to both because sometimes the request ends and sometimes it closes and that just doesn't make sense in my mind. Those aren't mutually exclusive things in my experience. In my mind a request could close, causing it to end almost immediately afterwards. Or the request might screw up and not close properly, but it would still end. Or I can even imagine a situation where the request connection might be persistent and almost never close, but you'd still get your response and your request would still have an end.

Koichi Kobayashi koichik referenced this issue October 20, 2011
Closed

streams2 #1681

Koichi Kobayashi koichik closed this October 21, 2011
Koichi Kobayashi
Collaborator

Closed by de09168. Thanks for the report.

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.