Listen for end events #36

kevinburke · 2018-10-04T23:00:47Z

In the event that the read stream terminates before all of the data
has been read, the pipe will send an 'end' event. We should listen for
this event and hit the callback with it, instead of hanging forever.

Added tests to verify this. Verified the patch fixes the problem by
running the tests without the patch, observing that they timeout, then
running the tests with the patch and verifying you get an error back.

jsf-clabot · 2018-10-04T23:00:48Z

All committers have signed the CLA.

codecov · 2018-10-05T00:19:25Z

Codecov Report

Merging #36 into master will not change coverage.
The diff coverage is 0%.

@@         Coverage Diff          @@
##           master   #36   +/-   ##
====================================
  Coverage       0%    0%           
====================================
  Files           7     7           
  Lines         372   384   +12     
  Branches       66    68    +2     
====================================
- Misses        325   335   +10     
- Partials       47    49    +2

Impacted Files	Coverage Δ
src/readBuffer.js	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e07fe0e...326045c. Read the comment docs.

The thread-loader package starts several subprocesses. When you send a SIGINT to thread-loader, the subprocesses terminate, but the parent process hangs while waiting for them to send data. We were not listening for the 'end' event in the parent process which meant that the webpack processes would just hang forever. This is a problem because this blocks the shutdown of goreman which means we can have orphan processes and is, in general, a huge hassle when developing the codebase locally. Fix this by listening for an 'end' event, which means we can signal to Webpack that the job terminated with an error, instead of not terminating. This means that SIGINT will actually shut down the process instead of leaving it hanging forever. Fixes sourcegraph/sourcegraph#186. Updates webpack/thread-loader#33. Updates webpack/thread-loader#34. Updates webpack/thread-loader#35. Updates webpack/thread-loader#36.

mistic · 2018-12-12T21:32:28Z

@evilebottnawi you review this please? I have also found this issue in a simple use case #39

alexander-akait

Other look good. Thanks!

alexander-akait · 2018-12-13T09:11:19Z

.travis.yml

      before_script: npm i --no-save git://github.com/webpack/webpack.git#master
      script: npm run travis:$JOB_PART
-      node_js: 8
+      node_js: 10


I think canary tests should be on minimum supported node, i.e. 6

mistic · 2018-12-13T14:01:14Z

@kevinburke can you check the review and change accordingly please?

alexander-akait · 2018-12-13T14:02:53Z

@kevinburke Also please accept CLA

kevinburke · 2018-12-13T23:02:45Z

I've signed the CLA.

mistic · 2018-12-14T03:41:30Z

@kevinburke the build for this PR should become green after merging #44

After it gets merged, can you merge this branch with master so you can have a green CI before merging it?

alexander-akait · 2018-12-14T09:27:26Z

Need rebase, feel free to ping me when it is done or recreate the PR

Attempting to run "npm install" on a new-ish Mac with Node v10.11 fails with a number of node-gyp compilation failures. Installing and using the latest versions of node-sass, nan, and fsevents resolves this failure, as each of those packages resolves the issues that led to problems. Update lodash to v4.17.11 to fix a security warning present in the Travis CI configuration. Add Node 10 to the build matrix so we can test for it. I am nervous about the number of changes to package-lock.json and I am not sure how to mitigate that - apparently NPM 6 changed the way requirements are stored in the file so we'll just have to do this at some point. npm/npm#20434

In the event that the read stream terminates before all of the data has been read, the pipe will send an 'end' event. We should listen for this event and hit the callback with it, instead of hanging forever. Added tests to verify this. Verified the patch fixes the problem by running the tests without the patch, observing that they timeout, then running the tests with the patch and verifying you get an error back.

kevinburke · 2018-12-14T11:17:32Z

I rebased but I'm not happy as I am not sure I can verify the changes in package-lock.json are correct.

I submitted a change that just runs "dedupe" on its own, once that is merged maybe the diff here will be clearer.

alexander-akait · 2018-12-14T11:24:37Z

@kevinburke you should not care about package-lock.json

alexander-akait · 2018-12-14T11:25:19Z

@kevinburke

                    === npm audit security report ===                        
                                                                                
# Run  npm update cryptiles --depth 9  to resolve 3 vulnerabilities
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Insufficient Entropy                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ cryptiles                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ jest [dev]                                                   │
├───────────────┼─────────────────────���────────────────────────────────────────┤
│ Path          │ jest > jest-cli > jest-config > jest-environment-jsdom >     │
│               │ jsdom > request > hawk > cryptiles                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/720                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Insufficient Entropy                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ cryptiles                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ jest [dev]                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ jest > jest-cli > jest-environment-jsdom > jsdom > request > │
│               │ hawk > cryptiles                                             │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/720                       │
└───────────────┴─────────────────���────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Insufficient Entropy                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ cryptiles                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ jest [dev]                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ jest > jest-cli > jest-runtime > jest-config >               │
│               │ jest-environment-jsdom > jsdom > request > hawk > cryptiles  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/720                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
[!] 3 vulnerabilities found - Packages audited: 12018 (12010 dev, 291 optional)
    Severity: 3 High

Just send a PR with updating deps

alexander-akait · 2018-12-14T11:26:19Z

Try rm -r package-lock.json && rm -r node_modules && npm i

kevinburke · 2018-12-14T11:31:53Z

I don’t have the time to audit every dependency update for correctness at the moment, unfortunately. As I understand it dedupe does not change dependency versions.

On Fri, Dec 14, 2018 at 11:26 Evilebot Tnawi ***@***.***> wrote: Try rm -r package-lock.json && rm -r node_modules && npm i — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOSI44X2G7sufZc9RSyG2RwyFdpLCeUks5u44rbgaJpZM4XJNWF> .

-- Kevin Burke phone: 925.271.7005 | kev.inburke.com

alexander-akait · 2018-12-14T11:36:07Z

@kevinburke no need audit dependencies just update this deps and all, need update jest and all

kevinburke · 2018-12-14T11:52:45Z

Apologies but as the combined-stream security issue last week demonstrated, blindly updating every dependency without understanding what they do and what’s changing is not a great philosophy nor is it one I agree with. I audited the two packages I updated here. You are welcome to update every dep separately but I don’t want my name attached to a commit that blindly updates deps and introduces a security vulnerability or a defect.

On Fri, Dec 14, 2018 at 11:36 Evilebot Tnawi ***@***.***> wrote: @kevinburke <https://github.com/kevinburke> no need audit dependencies just update this deps and all — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOSI9fJaooDkZdUl5jX07v1Rm3LmKl2ks5u440ogaJpZM4XJNWF> .

-- Kevin Burke phone: 925.271.7005 | kev.inburke.com

mistic · 2018-12-14T14:20:59Z

@kevinburke I think it's not realistic to think we can manually audit every dependency from our 3rd party dependencies as a project grows. What we should do, as in any matter of security, is to adopt the best standard procedures which I think we are already doing here running the npm audit in every build. The package.json was regenerated and audited just using the automated npm tasks npm install && npm audit fix

mistic · 2018-12-14T17:37:01Z

@kevinburke I've opened a PR to bump the old used dependencies and just run npm audit fix #47.

alexander-akait · 2018-12-17T11:16:37Z

Need rebase

alexander-akait · 2018-12-18T08:34:56Z

Done in #42, thanks for PRs and thanks for help

kevinburke force-pushed the listen-for-end-event branch from 585ba62 to fe6bb2d Compare October 5, 2018 00:19

kevinburke force-pushed the listen-for-end-event branch 6 times, most recently from 0579453 to ea7267d Compare October 5, 2018 17:23

kevinburke mentioned this pull request Oct 5, 2018

Point thread-loader at Sourcegraph fork sourcegraph/sourcegraph-public-snapshot#299

Merged

mistic mentioned this pull request Dec 12, 2018

EPIPE: broken pipe and sometimes ERR_STREAM_DESTROYED when Sending Ctrl+C (SIGINT) #39

Closed

alexander-akait requested changes Dec 13, 2018

View reviewed changes

alexander-akait closed this Dec 13, 2018

alexander-akait reopened this Dec 13, 2018

This was referenced Dec 13, 2018

Listen end events and small update over all the package #40

Closed

Listen end events #41

Closed

Listen end events #42

Merged

kevinburke force-pushed the listen-for-end-event branch 2 times, most recently from 7939a4f to e34202a Compare December 13, 2018 23:25

mistic mentioned this pull request Dec 14, 2018

Replace nsp with npm audit #44

Merged

This was referenced Dec 14, 2018

Fix compilation on Node v10.11 #34

Closed

SIGINT causes hang, callbacks never hit #33

Closed

Development dependencies don't compile with Node v10.11 #35

Closed

kevinburke force-pushed the listen-for-end-event branch 3 times, most recently from d9d54db to 63cc203 Compare December 14, 2018 11:08

kevinburke added 2 commits December 14, 2018 11:08

kevinburke force-pushed the listen-for-end-event branch from 63cc203 to 326045c Compare December 14, 2018 11:08

alexander-akait closed this Dec 18, 2018

Uh oh!

Listen for end events #36

Listen for end events #36

Uh oh!

Conversation

kevinburke commented Oct 4, 2018

Uh oh!

jsf-clabot commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mistic commented Dec 12, 2018

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-akait Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

kevinburke Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

mistic commented Dec 13, 2018

Uh oh!

alexander-akait commented Dec 13, 2018

Uh oh!

kevinburke commented Dec 13, 2018

Uh oh!

mistic commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait commented Dec 14, 2018

Uh oh!

kevinburke commented Dec 14, 2018

Uh oh!

alexander-akait commented Dec 14, 2018

Uh oh!

alexander-akait commented Dec 14, 2018

Uh oh!

alexander-akait commented Dec 14, 2018

Uh oh!

kevinburke commented Dec 14, 2018 via email

Uh oh!

alexander-akait commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinburke commented Dec 14, 2018 via email

Uh oh!

mistic commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mistic commented Dec 14, 2018

Uh oh!

alexander-akait commented Dec 17, 2018

Uh oh!

alexander-akait commented Dec 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jsf-clabot commented Oct 4, 2018 •

edited

Loading

codecov bot commented Oct 5, 2018 •

edited

Loading

mistic commented Dec 14, 2018 •

edited

Loading

alexander-akait commented Dec 14, 2018 •

edited

Loading

mistic commented Dec 14, 2018 •

edited

Loading