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

give more time to workers to finish the process #89

Merged
merged 10 commits into from
Dec 21, 2018
Merged

Conversation

byara
Copy link
Contributor

@byara byara commented Dec 4, 2018

This PR gives more time to the workers to finish before ending them.

@liangchunn
Copy link

I think... we need a better way 😛

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@frvge
Copy link

frvge commented Dec 4, 2018

@efegurkan , what do you think?

@coveralls
Copy link

coveralls commented Dec 4, 2018

Coverage Status

Coverage decreased (-0.3%) to 86.897% when pulling 9f8edc8 on byara:master into a07e303 on trivago:master.

@efegurkan
Copy link
Contributor

IMO, this is a hack that should be solved better. You should find a way to add this timeout as a flag, config etc. otherwise it will be a breaking change.

index.js Outdated
workerFarm.end(workers);
process.removeListener('SIGINT', shutdownCallback);
}).finally(function() {
injectTimeout(finallCallback, 10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does a smaller number work as well? Is there any real signal that we can hook into to find out the right timeout?

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 believe getting a result with any number is not guaranteed. we can try smaller numbers though.
For a signal to hook into, I think we should use the new plugin API. The done hook would be the best choice, I presume.

Choose a reason for hiding this comment

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

This plugin is already listening to the done hook. The issue is that webpack-bundle-analyzer is set up wrongly internally, and only executing after the done hook is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pago I checked different values for the timeout and it seems it depends on the size of the stats file. I'll try to add the timeout as a flag or config.

@liangchunn
Copy link

liangchunn commented Dec 5, 2018

@pago
To be honest, I think this problem is related to how webpack and webpack-bundle-analyzer interacts, and this PR will just 'work around' the issues that @byara and some other people have encountered.

From what I've seen in the source of webpack-bundle-analyzer, it defers the job of actually analyzing and emitting the stats by utilizing setImmediate(() => {}) here, and then implicitly returns.

That said, webpack actually doesn't know that there's enqueued operations, since webpack-bundle-analyzer only does the transformations after I/O operations are done, which at the same time, webpack might have already declared it's job done, and calling its done callback, and on the parallel-webpack side terminating the worker farm.

This 'hack' works because the farm termination is effectively delayed, and the underlying processes are essentially allowed to do whatever they want (and yes, probably executing the enqueued operations that webpack-bundle-analyzer has set).

10 seconds should be sufficient, but imagine a hypothetical situation where a huge stats.json cannot be parsed under 10 seconds: this issue will still persist.

I might do a little bit of research/test into this again since it's pretty interesting to me why webpack-bundle-analyzer uses setImmediate rather than a synchronous call.

@byara byara changed the title give more time to finish the process give more time to workers to finish the process Dec 11, 2018
bin/run.js Outdated
@@ -47,6 +47,7 @@ if(argv.version) {
exclude: argv['display-exclude'],
colors: argv['colors'],
stats: argv['stats'],
timeout: argv['timeout'],
Copy link
Contributor

Choose a reason for hiding this comment

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

@pago
Copy link
Contributor

pago commented Dec 18, 2018

@byara @liangchunn Last thing I'm still wondering about is the name of the flag. timeout indicates to me that the build will stop after that time but here its used to describe a mandatory delay.
How about keep-alive-after-finish or something similar?

@byara
Copy link
Contributor Author

byara commented Dec 18, 2018

@byara @liangchunn Last thing I'm still wondering about is the name of the flag. timeout indicates to me that the build will stop after that time but here its used to describe a mandatory delay.
How about keep-alive-after-finish or something similar?

absolutely right. I have no problem with that. If we cannot think of something shorter, I will make the changes according to keep-alive-after-finish.

@pago pago merged commit 66da53e into trivago:master Dec 21, 2018
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.

6 participants