Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Handle graceful termination of workerpool for parallel builds #953

Merged
merged 13 commits into from
Nov 17, 2023
27 changes: 22 additions & 5 deletions lib/processors/minifier.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import workerpool from "workerpool";
import Resource from "@ui5/fs/Resource";
import {getLogger} from "@ui5/logger";
const log = getLogger("builder:processors:minifier");
import {setTimeout as setTimeoutPromise} from "node:timers/promises";

const debugFileRegex = /((?:\.view|\.fragment|\.controller|\.designtime|\.support)?\.js)$/;

Expand All @@ -28,11 +29,27 @@ function getPool(taskUtil) {
workerType: "auto",
maxWorkers
});
taskUtil.registerCleanupTask(() => {
log.verbose(`Terminating workerpool`);
const poolToBeTerminated = pool;
pool = null;
poolToBeTerminated.terminate();
d3xter666 marked this conversation as resolved.
Show resolved Hide resolved
taskUtil.registerCleanupTask((force) => {
const attemptPoolTermination = async () => {
log.verbose(`Attempt to terminate the workerpool...`);

if (!pool) {
return;
}

// There are many stats that could be used, but these ones seem the most
// convenient. When all the (available) workers are idle, then it's safe to terminate.
const {idleWorkers, totalWorkers} = pool.stats();
while (idleWorkers !== totalWorkers && !force) {
await setTimeoutPromise(100); // Wait a bit workers to finish and try again
}

const poolToBeTerminated = pool;
pool = null;
return poolToBeTerminated.terminate(force);
};

return attemptPoolTermination();
});
}
return pool;
Expand Down
27 changes: 22 additions & 5 deletions lib/tasks/buildThemes.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {fileURLToPath} from "node:url";
import os from "node:os";
import workerpool from "workerpool";
import {deserializeResources, serializeResources, FsMainThreadInterface} from "../processors/themeBuilderWorker.js";
import {setTimeout as setTimeoutPromise} from "node:timers/promises";

let pool;

Expand All @@ -23,11 +24,27 @@ function getPool(taskUtil) {
workerType: "thread",
maxWorkers
});
taskUtil.registerCleanupTask(() => {
log.verbose(`Terminating workerpool`);
const poolToBeTerminated = pool;
pool = null;
poolToBeTerminated.terminate();
taskUtil.registerCleanupTask((force) => {
const attemptPoolTermination = async () => {
log.verbose(`Attempt to terminate the workerpool...`);

if (!pool) {
return;
}

// There are many stats that could be used, but these ones seem the most
// convenient. When all the (available) workers are idle, then it's safe to terminate.
const {idleWorkers, totalWorkers} = pool.stats();
d3xter666 marked this conversation as resolved.
Show resolved Hide resolved
while (idleWorkers !== totalWorkers && !force) {
await setTimeoutPromise(100); // Wait a bit workers to finish and try again
}

const poolToBeTerminated = pool;
pool = null;
return poolToBeTerminated.terminate(force);
};

return attemptPoolTermination();
});
}
return pool;
Expand Down
37 changes: 37 additions & 0 deletions test/lib/processors/minifier.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,43 @@ ${SOURCE_MAPPING_URL}=test.controller.js.map`;
t.is(taskUtilMock.registerCleanupTask.callCount, 1, "taskUtil#registerCleanupTask got called once");
});

test("Basic minifier with taskUtil and unexpected termination of the workerpool", async (t) => {
const taskUtilMock = {
registerCleanupTask: sinon.stub().callsFake((cb) => {
// Terminate the workerpool in a timeout, so that
// the task is already in the queue, but not yet finished.
setTimeout(cb);
})
};

// Create more resources so there to be some pending tasks in the pool
const testResources = [];
for (let i = 0; i < 50; i++) {
const content = `/*!
* \${copyright}
*/
function myFunc${i}(myArg) {
jQuery.sap.require("something");
console.log("Something required")
}
myFunc${i}();
`;
testResources.push(createResource({
path: "/test.controller.js",
string: content
}));
}
await minifier({
resources: testResources,
taskUtil: taskUtilMock,
options: {
useWorkers: true
}
});

t.pass("No exception from an earlier workerpool termination attempt.");
});

test("minifier with useWorkers: true and missing taskUtil", async (t) => {
const content = `/*!
* \${copyright}
Expand Down
62 changes: 62 additions & 0 deletions test/lib/tasks/buildThemes.js
Original file line number Diff line number Diff line change
Expand Up @@ -549,3 +549,65 @@ test.serial("buildThemes (useWorkers = true)", async (t) => {
t.true(workspace.write.calledWithExactly(transferredResources[1]));
t.true(workspace.write.calledWithExactly(transferredResources[2]));
});


test.serial("buildThemes with taskUtil and unexpected termination of the workerpool", async (t) => {
const taskUtilMock = {
registerCleanupTask: sinon.stub().callsFake((cb) => {
// Terminate the workerpool in a timeout, so that
// the task is already in the queue, but not yet finished.
setTimeout(cb);
})
};
const lessResources = [];

// Create more resources so there to be some pending tasks in the pool
for (let i = 0; i < 50; i++) {
lessResources.push({
getPath: () => `/resources/test${i}/themes/${i}/library.source.less`,
getBuffer: () => Buffer.from(`/** test comment N ${i} */`),
});
}

const workspace = {
byGlob: async (globPattern) => {
if (globPattern === "/resources/test*/themes/**/library.source.less") {
return lessResources;
} else {
return [];
}
},
write: sinon.stub()
};

const cssResource = {path: "/cssResource", buffer: new Uint8Array(2)};
const cssRtlResource = {path: "/cssRtlResource", buffer: new Uint8Array(2)};
const jsonParametersResource = {path: "/jsonParametersResource", buffer: new Uint8Array(2)};

t.context.themeBuilderStub.returns([cssResource, cssRtlResource, jsonParametersResource]);
t.context.comboByGlob.resolves(lessResources);

t.context.fsInterfaceStub.returns({
readFile: (...args) => {
if (/\/resources\/test.*\/themes\/.*\/library\.source\.less/i.test(args[0])) {
args[args.length - 1](null, "/** */");
} else {
args[args.length - 1](null, "{}");
}
},
stat: (...args) => args[args.length - 1](null, {}),
readdir: (...args) => args[args.length - 1](null, {}),
mkdir: (...args) => args[args.length - 1](null, {}),
});

await buildThemes({
workspace,
taskUtil: taskUtilMock,
options: {
projectName: "sap.ui.demo.app",
inputPattern: "/resources/test*/themes/**/library.source.less"
}
});

t.pass("No exception from an earlier workerpool termination attempt.");
});