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

Avoid 'data' and/or 'end' events after pause() was called #1040

Closed
wants to merge 1 commit into from
Closed

Avoid 'data' and/or 'end' events after pause() was called #1040

wants to merge 1 commit into from

Conversation

koichik
Copy link

@koichik koichik commented May 12, 2011

In http.Server, if I called request.pause() but the request emits
'data' and/or 'end' event.

Reproduce:

$ cat server.js
var http = require('http');
http.createServer(function(request, response) {
  request.pause();
  request.on('end', function() {
    console.log('after pause(), but called');
  });
  process.nextTick(function() {
    request.removeAllListeners('end');
    request.resume();
    request.on('end', function() {
      console.log('after resume(), but not called');
    });
    response.end('hello world');
  });
}).listen(8124, '127.0.0.1');

$ node server.js
after pause(), but called

Because the request body has been received with the headers.
This patch suspends an event when stream was paused.

If the patch was landed, the following problem is solved
by simple way.

http://groups.google.com/group/nodejs/browse_thread/thread/6e7bc2508abd1717#

http.createServer(function (request, response) {
  request.pause();
  var path = ...;
  path.exists(path, function(exists) {
    if (exists) {
      request.resume();
      request.pipe(new MyStream());
    }
  });
}).listen(port, adress);

This change is the same as:
http://groups.google.com/group/nodejs-dev/browse_thread/thread/7bafdcbd119b619e#

@ry
Copy link

ry commented May 14, 2011

This is something that we need to land in master instead of v0.4. I don't want to do it right now because of the large flux due to libuv changes. I will revisit in a month.

@sh1mmer
Copy link

sh1mmer commented Oct 25, 2011

Any update on this?

@bnoordhuis
Copy link
Member

@koichik: Can you rebase your patch and land it in master?

@koichik
Copy link
Author

koichik commented Dec 26, 2011

@bnoordhuis Thanks, merging.

@koichik koichik closed this in e6b6075 Dec 26, 2011
@strager
Copy link

strager commented Jan 26, 2012

Is this going into a stable release (v0.6.9?) any time soon?

@koichik
Copy link
Author

koichik commented Jan 26, 2012

@strager - Sorry, no. Because this changed the behavior rather than a bug fix.

@strager
Copy link

strager commented Jan 27, 2012

@koichik Okay, that's reasonable. I guess I'll stick to Node 0.7.1 for now.

I don't see how it's a breaking change; the way I see it, .resume simply wasn't working before.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants