Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Conversation

@kevinburke
Copy link
Contributor

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.

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.

Updates webpack/thread-loader#33.
Updates webpack/thread-loader#34.
Updates webpack/thread-loader#35.
Updates webpack/thread-loader#36.

This PR does not need to update the CHANGELOG because it's not really user
facing.

Copy link
Contributor

@ijsnow ijsnow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably would have went ahead and published this to @sourcegraph/thead-loader on npm but this is fine with me.

package.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does a git reference work? If I look at package.json, the main field points to dist/cjs.js, which is not checked into git since its compiled

https://github.com/webpack-contrib/thread-loader/blob/c3366d61cf4621f37483e4e7a1287783eb70277b/package.json#L7

Copy link
Contributor

@ijsnow ijsnow Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering the same thing but CI passed so I looked and the dist directory is checked in. Not best practices but from my understanding, this is temporary. The dist is pretty small.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed another change which checks in the dist folder, it seemed like the easiest way to resolve the issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe thread-loader is not used in CI builds and only in dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - The commit referenced in the build there will install a version of the package that has the dist field present.

@kevinburke
Copy link
Contributor Author

I probably would have went ahead and published this to @sourcegraph/thead-loader on npm but this is fine with me.

If you want to do this that would be great, I'm not sure I have access.

@kevinburke kevinburke force-pushed the update-thread-loader branch 3 times, most recently from 37b5a23 to 48142aa Compare October 5, 2018 20:44
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.
@kevinburke kevinburke force-pushed the update-thread-loader branch from 48142aa to b107ecf Compare October 5, 2018 21:08
@kevinburke kevinburke merged commit b107ecf into master Oct 5, 2018
@felixfbecker felixfbecker deleted the update-thread-loader branch October 5, 2018 21:25
BolajiOlajide pushed a commit that referenced this pull request Apr 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants