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

Fork Worker only if Cluster Initialized #111

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 43 additions & 14 deletions src/fastboot-app-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class FastBootAppServer {
this.sandboxGlobals = options.sandboxGlobals;
this.chunkedResponse = options.chunkedResponse;

this._clusterIsInitialized = false;

if (!this.ui) {
let UI = require('./ui');
this.ui = new UI();
Expand Down Expand Up @@ -64,12 +66,21 @@ class FastBootAppServer {
start() {
if (cluster.isWorker) { return; }

return this.initializeApp()
const forkWorkersPromise = this.initializeApp()
.then(() => this.subscribeToNotifier())
.then(() => this.forkWorkers())
.then(() => {

// fail hard if first boot fails
forkWorkersPromise.catch((error) => {
this.ui.writeLine(error);
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't really like the usage of process.exit here 🤔

});

return forkWorkersPromise.then(() => {
if (this.initializationError) {
this.broadcast({ event: 'error', error: this.initializationError.stack });
} else {
this._clusterIsInitialized = true;
}
})
.catch(err => {
Expand Down Expand Up @@ -163,28 +174,46 @@ class FastBootAppServer {
let env = this.buildWorkerEnv();
let worker = cluster.fork(env);

let firstBootResolve;
let firstBootReject;
const firstBootPromise = new Promise((resolve, reject) => {
firstBootResolve = resolve;
firstBootReject = reject;
});
Comment on lines +177 to +182
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit odd, can you refactor to not capture the resolve/reject and instead move the worker.on code inside the promise callback?


this.ui.writeLine(`forked worker ${worker.process.pid}`);

worker.on('online', () => {
this.ui.writeLine('worker online');

worker.on('message', message => {
if (message.event === 'http-online') {
firstBootResolve();
}
});
});

worker.on('exit', (code, signal) => {
let error;

if (signal) {
this.ui.writeLine(`worker was killed by signal: ${signal}`);
error = new Error(`Worker ${(worker.process.pid)} was killed by signal: ${signal}`)
} else if (code !== 0) {
this.ui.writeLine(`worker exited with error code: ${code}`);
error = new Error(`Worker ${(worker.process.pid)} exited with an error code: ${code}`)
} else {
this.ui.writeLine(`worker exited`);
error = new Error(`Worker ${(worker.process.pid)} exited gracefully`)
}

this.forkWorker();
if (!this._clusterIsInitialized) {
// dont attempt to fork again if never a healthy first boot
firstBootReject(error);
} else {
this.ui.writeLine(error);
this.forkWorker();
}
});

return new Promise(resolve => {
this.ui.writeLine('worker online');
worker.on('message', message => {
if (message.event === 'http-online') {
resolve();
}
});
});
return firstBootPromise;
}

buildWorkerEnv() {
Expand Down