set 1s delay for untar pipes to avoid spawn racing

[imcotton]

In previous PR #33, the outdated dependency pump ¹ has been replace by native streams pipeline ², which intended to be drop-in replacement given ported by its original author (nodejs/node#19828).

It seems to flaky for some Docker related setups (i.e. computing resources intensive, caching), since it has a lot of moving parts (node & npm versions) involved, I'd like to patch with this minimal changes by having 1 second delay before the untar pipe proceed to its next tasks.

To preview & verify, for Node.js version >= v16 (shipped with npm v8) could use the overrides syntax to select this patch by modify the package.json:

  },
+ "overrides": {
+   "purescript-installer": "github:imcotton/npm-installer#untar-pipe-racing-delay"
+ },
  "devDependencies": {
    "purescript": "~0.15.6"
  },

For Node.js v14 consider adding an extra step in Dockerfile to upgrade builtin npm to v8 first

FROM node:14
RUN npm install -g npm@8

---

resolves #37

/cc @mrskug @sjpeterson @rhendric

Footnotes

1. https://github.com/mafintosh/pump ↩

2. https://nodejs.org/docs/latest-v14.x/api/stream.html#stream_stream_pipeline_streams_callback ↩

[imcotton]

haskell/actions#128

[imcotton]

Hi, I've created one verify repo with this patch opt-in and CI ready, feel free to take a look if you're interested.

https://github.com/imcotton/demo-ci-ps-npm-installer-untar-delay

[imcotton]

Hi @JordanMartinez, I have another minor PR #39 opened, should I keep these two separated or maybe just cherry-pick the commit over here?

[JordanMartinez]

Keep them separate.

I don't think this fixes the root cause of the issue.

[JordanMartinez]

What is the root cause that causes this issue and does this PR solve that issue? If 1s was chosen arbitrarily just to make things work in this environment, I'm guessing it won't work in other environments where perhaps 2 seconds was needed. In other words, the root issue is still unresolved. This is additional feedback I'm providing after talking to Nate about this issue. Hence, why I dismissed my earlier review.

[rhendric]

I was doing a little sniffing around here to see if I can figure it out. I can say that if everything in #33 is kept except for the dependency change from request to make-fetch-happen and the associated code changes in our vendored dl-tar, the issue goes away. However, the issue comes back if I use request but keep the current code pattern in dl-tar, starting the pipeline only once the response is received instead of before the request is issued.

I'm not sure what to make of this yet; there does seem to be a whoooooole lot of buggy code floating around dealing with how streams work, both in the Node internals at various versions and in the NPM ecosystem at large, not to mention a fair amount of confusion between the different types of streams that Node started supporting at different points in time.

Reverting back to request from make-fetch-happen might be the preferred quick fix, though request does raise a lot of audit flags these days and that would probably invalidate much of the motivation behind #33.

Edit: We could also vendor request and rip out all of its dependencies; we use a very small part of that library. The redirect handling is really the only interesting part.

[imcotton]

1 second has whole a lot of margin here, I suspect it will be good enough even by setTimeout(_, 0) which is the old trick to shift run to completion code to the next batch of event loop.

To my understanding, at the worse case this change only adding 1 second delay to the installation process, thus I'd like to get some feedback from @mrskug and @sjpeterson on the patch as cross verify.

A side from baggage of this code base itself, this is a tricky issue if you'd like to digging into the root, which involving: scheduling between microtasks & macrotasks, back-pressure of stream piping, non-blocking I/O to the kernel, etc..., which I'm out bandwidth at the moment unfortunately.

[rhendric]

Okay, I got it. The package download and extract is being triggered twice with the new code, because there's a tmpSubscription hack inside install-purescript that starts and then cancels a download (this is in addition to the main download invocation). The old code pattern was more responsive to the cancellation than the new one with fetch; the error was caused by the two download-and-extracts racing each other, with one in the process of overwriting the purs binary after the other finished and triggered an execution of purs --version.

I have no idea what the tmpSubscription thing is for. I'd go spelunking through the Git history but the vendoring of everything in this project made that impossible. Ripping it out seems to get rid of the problem and I haven't noticed any adverse side effects.

See #40.

[rhendric]

Someone close this?