Fix build hang on Windows 10 #23589
Fix build hang on Windows 10 #23589
Conversation
ramonjd
added
[Type] Build Tooling
There was a problem hiding this comment.
This seems like a sensible change to take the increment operator out of the if condition.
I don't have a Windows machine, but npm run build:packages build still work for me.
I think we can merge this. cc @aduth @youknowriad who last edited this file just to double check.
|
I'm not familiar particularly with that code but it seems to make sense, can we rebase maybe to get all the tests passing. |
|
I tried to rebase this, but I can't get the build running at all due to #34377 I'll try again once that issue is cleared. |
On Windows 10, I seem to be unable to build. The build completion rises to 100%, but the process never finishes. I can build if I manually set the number of workers in the farm to 4, but not if it's any higher. It appears that some files are not being counted as "complete". It's probably files that finish before the "ended" signal is recieved (meaning everything has been queued.) Moving the increment out of the conditional fixes this.
SeanMcMillan
force-pushed
the
fix-build
branch
from
45dedaf
to
8048064
Compare
|
Rebased. I can't always reproduce the hang without this change; It seems to be dependent on timing. I can never reproduce the hang with this change. |
There was a problem hiding this comment.
I can't reproduce the issue, but the problem and solution make sense. The workerFarm instance is meant to be ended when there's no more data to process and all files have completed, but complete would not be incremented as long as ended is false due to boolean short-circuiting. Not sure if that's a Windows thing, or just a CPU/IO race condition thing, which might explain why it only happens sometimes.
Alternatively, the order of the two conditions could be swapped ( ++complete === files.length && ended ) and I expect it would fix the issue as well, though it seems clearer to pull it out as you've proposed.
@youknowriad would you mind giving this a quick confidence check before merging? Since I've not been following the project closely, I'm not sure if there might be some potential conflict to consider here, though I don't expect it'd be an issue.
|
Thanks for chiming in here @aduth :) this makes sense to me as well. |
|
Congratulations on your first merged pull request, @SeanMcMillan! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
On Windows 10, I seem to be unable to build. The build completion rises to 100%, but the process never finishes. I can build if I manually set the number of workers in the farm to 4, but not if it's any higher.
It appears that some files are not being counted as "complete". It's probably files that finish before the "ended" signal is received (meaning everything has been queued.) Moving the increment out of the conditional fixes this.
Description
Fix build hang on Windows 10
How has this been tested?
The build would hang on my Windows 10 machine, but succeed on my Mac. Making this change causes the build to succeed on my Windows 10 machine.
Types of changes
Bug Fix (Build tools)
Checklist: