Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

fix pipe not emit finish on tarball in 0.10.30 #5958

Closed
wants to merge 1 commit into from

Conversation

dead-horse
Copy link
Contributor

in node v0.10.29 or v0.10.30, when the http response is not keepalive, this won't emit finish in downstream:

var http = require('http');
var fs = require('fs');
var path = require('path');
var tmp = path.join(__dirname, 't.tgz');

var opts = {
  hostname: 'registry.npmjs.com',
  method: 'GET',
  port: 80,
  path: '/cutter/-/cutter-0.0.3.tgz',
  headers: {
    Connection: 'close'
  }
}

http.request(opts, function (res) {
  console.log('connection: %s', res.headers.connection);
  res.resume();
  res.pause();
  setTimeout(function () {
    var tarball = fs.createWriteStream(tmp, { mode: 420 })
    tarball.on('finish', function () {
      console.log('tarball finish');
    })
    res.on('data', function (data) {
      console.log('res on data: %s', data.length);
    })
    res.on('end', function (d) {
      console.log('res end');
    })
    res.pipe(tarball);
    res.resume();
  }, 1000);

}).end();

// output:
// connection: close
// res on data: 3144
// res end

this is the logic in lib/cache/add-remote-tarball.js now. i think it is a bug in node. but npm should fix it anyway. :)

the more detail in these gists:

https://gist.github.com/dead-horse/c7030afa663fee61028b
https://gist.github.com/dead-horse/6705119f1a2d41c48899

@othiym23
Copy link
Contributor

Thanks for reporting this, and thanks for filing the issue on Node!

This is an irritating bug, but the proposed solution forces the res stream into flowing mode, and I'd like to see if we can find a less hacky workaround. Also (and i know I keep beating this drum) it would be really nice to have a test, even if it only applies to a few specific versions of Node. I've asked @isaacs to take a look at this.

@othiym23
Copy link
Contributor

There's also a possibility that addressing #5926 will address this issue as well.

@fengmk2
Copy link
Contributor

fengmk2 commented Aug 15, 2014

I think we should mkdir before the registry.fetch response.

@dead-horse
Copy link
Contributor Author

actually it seems like already mkdir in https://github.com/npm/npm/blob/master/lib/cache/add-remote-tarball.js#L51

@dead-horse
Copy link
Contributor Author

i update the PR. now it just remove the repeating mkdir. it can avoid this bug temporary.

@othiym23
Copy link
Contributor

Landed in 16bead7. Thanks, @dead-horse! We also discovered that fstream is generally pretty mkdirp-happy in the process of trying to write a test for this, but since this doesn't break any of the existing tests, that's good enough for now.

@othiym23 othiym23 closed this Aug 21, 2014
@dead-horse dead-horse deleted the pipe-finish branch August 22, 2014 01:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants