zkat / pacote

programmatic npm package and metadata downloader (moved!)
https://github.com/npm/pacote
MIT License
280 stars 62 forks source link

spurious ENOVERSIONS on git dependencies #143

Closed andreineculau closed 6 years ago

andreineculau commented 6 years ago

we've been seeing random failures both on our CI and our dev machines that didn't make sense. npm was failing to see any valid versions for a random git dependency in the tree

I spent some hours investigating the issue, and within the scope of pacote, it boils down to an issue with when finished ends up resolving the promise, and that is "too soon", as in I observe stdout and stderr being empty strings when execution reaches REVS.set(repo, revs) on https://github.com/zkat/pacote/blob/latest/lib/util/git.js#L178

The way that I isolated the issue was by replacing finished with a naïve onclose which does not end up with empty stdout or if it does, then it has a lower chance for some reason. I have not experienced any issues in a 100-times trial. I didn't move into debugging the end-of-stream package (=finished).

Naïve onclose replacement (I say naïve because I guess there are reasons why pacote uses end-of-stream, which I do not know of):

    /*return finished(child).catch(err => {
        err.message = `Error while executing:\n${GITPATH} ls-remote -h -t ${repo}\n\n${stderr}\n${err.message}`
        throw err
      }).then(() => {*/
      return (new Promise(function(resolve, reject) {
        child.on('error', reject).on('close', function(code) {
          if (code === 0) {
            resolve();
          } else {
            let err = new Error(`Error while executing:\n${GITPATH} ls-remote -h -t ${repo}\n\n${stderr}\n${err.message}`);
            reject(err);
          }
        })
      })).then(() => {

The 100-times trial was run with this package.json, executing in a git repo that has only this file checked in - git clean -xdf; npm i (npm@5.7.1)

{
  "private": true,
  "name": "lodash-firecloud",
  "version": "0.1.2",
  "dependencies": {
    "babel-preset-firecloud": "git://github.com/tobiipro/babel-preset-firecloud.git#semver:~0.1.2",
    "eslint-config-firecloud": "git://github.com/tobiipro/eslint-config-firecloud.git#semver:~0.1.2",
    "eslint-plugin-firecloud": "git://github.com/tobiipro/eslint-plugin-firecloud.git#semver:~0.1.0",
    "npm-publish-git": "git://github.com/andreineculau/npm-publish-git.git#semver:~0.0.7"
  }
}

The same package.json would fail with ENOVERSIONS on any of the -firecloud deps, maybe even npm-publish-git though I did not experience that in my trials.

Ref: internal reported issue with links to Travis CI runs https://github.com/tobiipro/support-firecloud/issues/17

zkat commented 6 years ago

This is really great sleuthing, thank you! And uuggghhh I've been noticing occasional failures while using mississippi recently, too -- I've even ended up rewriting sections of code using it in the same way you're talking about, precisely because there's something weird about its end detection that I haven't debugged either.

zkat commented 6 years ago

Would you be interested in putting together a patch for this (obvs getting the code to fit surrounding style and such)? Thanks again for figuring all this out and checking out that the solution you found actually works 👍

andreineculau commented 6 years ago

Yes, ofc I'm interested :) If I hear you correctly, you're saying that you may want to do away with mississippi.finished i.e. end-of-stream and "KISS" in all places, maybe even go extreme and remove the mississippi dependency altogether? Or do you want a surgical patch just for the git fetcher?

IanSavchenko commented 6 years ago

More git-dependencies we have, more often this issue occurs... Would be cool if this was finally resolved.