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

feat(child process): Make all child processes silent #1039

Merged
merged 65 commits into from
Aug 2, 2018
Merged
Show file tree
Hide file tree
Changes from 54 commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
01a9b36
feat(Logging api): add logging api
nicojs May 13, 2018
187855e
feat(logging-api): first usage of logging api
nicojs May 15, 2018
a6feb8c
Merge branch 'master' into 748-logging-api
nicojs May 26, 2018
8e50b4f
Update log4js 2.7
nicojs May 27, 2018
09eb2f2
Merge branch 'master' into 748-logging-api
nicojs Jul 1, 2018
c50d69d
feat: add file log level option to stryker config
nicojs Jul 6, 2018
98c2bfb
chore: add stryker.log to gitignore
nicojs Jul 6, 2018
c002d5c
docs(stryker): Document new file log level option
nicojs Jul 6, 2018
7d2f3df
feat(file logging): add logging server
nicojs Jul 6, 2018
6590624
refactor: remove unused import
nicojs Jul 6, 2018
691e3a7
test(logging): Add unit test for multi appender
nicojs Jul 6, 2018
042117e
refactor(stryker): Add semicolons
nicojs Jul 6, 2018
188bada
test(logging): Add unit tests for the log configurator
nicojs Jul 6, 2018
d5d483e
Merge branch 'master' into 748-logging-api
nicojs Jul 6, 2018
4fdc250
fix(karma-runner): remove logging logic from karma
nicojs Jul 6, 2018
1729794
build(dev depedencies): remove unused types for old log4js
nicojs Jul 6, 2018
c14a520
chore(karma-runner): Add express.js types back
nicojs Jul 6, 2018
0a30c08
test(stryker-api): fix failing test
nicojs Jul 6, 2018
5a92cc3
test(logging): Remove accidental `.only`
nicojs Jul 6, 2018
be5477f
fix(logging): Remove color from file layout
nicojs Jul 6, 2018
3339086
test(logging): Add logging integration test
nicojs Jul 6, 2018
10bed57
refactor(ConfigReader): don't use `process.exit`
nicojs Jul 6, 2018
a820577
fix(taskkill): make killing of tasks more relient on windows
nicojs Jul 6, 2018
ee1efda
test(api): Improve error logging on integration test
nicojs Jul 7, 2018
33aa4d9
test(karma-runner): Fix failing test in karma runner after merge
nicojs Jul 7, 2018
c2882b9
fix(logging): Make logging server more robust against race conditions
nicojs Jul 7, 2018
f59d0c2
test(stryker): Higher timeout for integration tests
nicojs Jul 7, 2018
9d6a4f6
refactor: Remove code responsible for retrying different ports as it …
nicojs Jul 7, 2018
82db388
Merge branch 'master' into 748-logging-api
nicojs Jul 12, 2018
145e583
feat(jest-runner): Use new logging API
nicojs Jul 12, 2018
d10b523
build(karma-runner): Add express types back again
nicojs Jul 12, 2018
cbe94cc
refactor(logging): Implement review comments
nicojs Jul 12, 2018
c37a500
refactor(mapped-types): Make use of conditional types to describe chi…
nicojs Jul 13, 2018
055cf87
Merge branch 'master' into 976-silent-testrunners
nicojs Jul 17, 2018
e1a5191
Temp commit
nicojs Jul 18, 2018
6054561
feat(ChildProcess): improve resilience
nicojs Jul 20, 2018
556b80f
test(ChildProcess): test crash scenarios
nicojs Jul 22, 2018
f6b9d91
refactor(TestRunners): Generalize child process logic
nicojs Jul 22, 2018
142e879
Merge branch 'master' into 976-silent-testrunners
nicojs Jul 22, 2018
f776825
Merge branch 'master' into 976-silent-testrunners
nicojs Jul 23, 2018
9e91a25
test(Test runner): Move unit tests to match source location
nicojs Jul 23, 2018
85c9d3a
fix(ChildProcessProxy): Don't wait forever on dispose in worker
nicojs Jul 24, 2018
9d62efe
build: Don't run integration tests in stryker.conf.js
nicojs Jul 24, 2018
b35cfe6
fix(ChildProcessTestRunnerDecorator): Make resilient against crashes
nicojs Jul 24, 2018
075f84f
refactor(RetryDecorator): Remove redundent retry logic on dispose
nicojs Jul 24, 2018
f060649
fix(RetryDecorator): Cleanup unused promise
nicojs Jul 24, 2018
06e9998
refactor(TimeoutDecorator): Add semicolon
nicojs Jul 24, 2018
dd2ee86
test(TestRunner): Add integration test for crashing test runner
nicojs Jul 24, 2018
cb63eab
refactor(Task): Refactor task to use `Promise.race`
nicojs Jul 24, 2018
4df4ae5
Merge branch 'master' into 976-silent-testrunners
nicojs Jul 25, 2018
61af88a
fix(Test runner): Improved error message when a test runner process w…
nicojs Jul 26, 2018
787e249
Merge branch 'master' into 976-silent-testrunners
nicojs Jul 27, 2018
86e7fe2
test(child-process): Await all messages (red build on node 10)
nicojs Jul 28, 2018
3caa27f
fix(child-process): Increase message size, so out-of-memory message i…
nicojs Jul 28, 2018
81cc92f
Merge branch 'master' into 976-silent-testrunners
nicojs Jul 30, 2018
60360e5
refactor(test runner): Cleanup global reference
nicojs Jul 30, 2018
16d29a9
refactor(ChildProcessProxy): Remove unnessesary variable
nicojs Jul 30, 2018
4d3150c
Merge branch 'master' into 976-silent-testrunners
nicojs Jul 31, 2018
44823ad
fix(child-process): Correctly type the proxy param (since TS 3.0)
nicojs Jul 31, 2018
36e8984
refactor(child-process): Remove unused statement
nicojs Jul 31, 2018
b99d21a
refactor(child-process): Improve test error message
nicojs Jul 31, 2018
364d2af
fix(child-process): Make sure to capture enough of stdout and stderr …
nicojs Jul 31, 2018
71154e8
test(string builder): Implement unit tests
nicojs Aug 1, 2018
5b0e941
fix(test): Remove `.only`
nicojs Aug 1, 2018
42d95a4
fix(child-process): Remove death by 1000 overloads
nicojs Aug 2, 2018
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
4 changes: 2 additions & 2 deletions packages/stryker/src/Sandbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import { RunResult, RunnerOptions } from 'stryker-api/test_runner';
import { File } from 'stryker-api/core';
import { TestFramework } from 'stryker-api/test_framework';
import { wrapInClosure, normalizeWhiteSpaces } from './utils/objectUtils';
import TestRunnerDecorator from './isolated-runner/TestRunnerDecorator';
import ResilientTestRunnerFactory from './isolated-runner/ResilientTestRunnerFactory';
import TestRunnerDecorator from './test-runner/TestRunnerDecorator';
import ResilientTestRunnerFactory from './test-runner/ResilientTestRunnerFactory';
import { TempFolder } from './utils/TempFolder';
import { writeFile, findNodeModules, symlinkJunction } from './utils/fileUtils';
import TestableMutant, { TestSelectionResult } from './TestableMutant';
Expand Down
14 changes: 14 additions & 0 deletions packages/stryker/src/child-proxy/ChildProcessCrashedError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import StrykerError from '../utils/StrykerError';

export default class ChildProcessCrashedError extends StrykerError {
constructor(
public readonly pid: number,
public readonly exitCode?: number,
message = `Child process exited unexpectedly (code ${exitCode === null ? 'unknown' : exitCode})`,
innerError?: Error) {
super(message, innerError);
Error.captureStackTrace(this, ChildProcessCrashedError);
// TS recommendation: https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
Object.setPrototypeOf(this, ChildProcessCrashedError.prototype);
}
}
191 changes: 155 additions & 36 deletions packages/stryker/src/child-proxy/ChildProcessProxy.ts
Original file line number Diff line number Diff line change
@@ -1,80 +1,114 @@
import * as os from 'os';
import { fork, ChildProcess } from 'child_process';
import { File } from 'stryker-api/core';
import { getLogger } from 'stryker-api/logging';
import { WorkerMessage, WorkerMessageKind, ParentMessage, autoStart, ParentMessageKind } from './messageProtocol';
import { serialize, deserialize } from '../utils/objectUtils';
import Task from '../utils/Task';
import { serialize, deserialize, kill, isErrnoException, padLeft } from '../utils/objectUtils';
import { Task, ExpirableTask } from '../utils/Task';
import LoggingClientContext from '../logging/LoggingClientContext';
import ChildProcessCrashedError from './ChildProcessCrashedError';
import OutOfMemoryError from './OutOfMemoryError';

export type ChildProxy<T> = {
[K in keyof T]: (...args: any[]) => Promise<any>;
type MethodPromised = { (...args: any[]): Promise<any> };

export type Promisified<T> = {
[K in keyof T]: T[K] extends MethodPromised ? T[K] : T[K] extends Function ? MethodPromised : () => Promise<T[K]>;
Copy link
Member

Choose a reason for hiding this comment

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

In what case would you cant to use T[K] extends MethodPromised ? T[K]? Can't you just always call back on T[K] extends Function ? MethodPromised?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you are getting at. There are 3 scenarios supported here. In case the method already returns a promise, it is left alone. In case the method returns something else, it is changed to returning a promise. In case it's not a method, it is mapped to a method returning a promise of that type. This is not perfect (still missing the type arguments in some cases), but is the best I could came up with.

Copy link
Member Author

@nicojs nicojs Jul 30, 2018

Choose a reason for hiding this comment

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

Ah I see what you mean. You mean change it to this:

export type Promisified<T> = {
  [K in keyof T]: T[K] extends Function ? MethodPromised : () => Promise<T[K]>;
};

The problem is that you don't really want to use MethodPromised if you don't have to. It doesn't help you with method types, converting it to (...args: any[]): Promise<any>. It would be awesome if we could keep the method signature, but I don't know how. I've created a stack overflow question for this:

https://stackoverflow.com/questions/51587576/using-generics-with-mapped-conditional-types-in-typescript

I would say: let's keep it like this for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow! We have an answer on stack overflow! Fast. It seems that it is possible using infer. The problem now is that we do not have support for rest type parameters yet. So needs to be implemented using the death by a thousand overloads syntax. Let's keep it like it is for now and improve once TS 3 is out.

// TODO: Implement using unknown and rest parameter type
// https://blogs.msdn.microsoft.com/typescript/2018/07/12/announcing-typescript-3-0-rc/#tuples-and-parameters
interface Func1<R> {
  (): R;
}
interface Func2<T1, R> {
  (arg: T1): R;
}
interface Func3<T1, T2, R> {
  (arg: T1, arg2: T2): R;
}
interface Func<R> {
  (...args: any[]): R;
}
interface PromisedFunc1<R> {
  (): Promise<R>;
}
interface PromisedFunc2<T1, R> {
  (arg: T1): Promise<R>;
}
interface PromisedFunc3<T1, T2, R> {
  (arg: T1, arg2: T2): Promise<R>;
}
interface PromisedFunc<R> {
  (...args: any[]): Promise<R>;
}

export type Promisified<T> = {
  [K in keyof T]: T[K] extends { (...args: any[]): Promise<any> } ? T[K] :
  T[K] extends Func3<infer T1, infer T2, infer R> ? PromisedFunc3<T1, T2, R> :
  T[K] extends Func2<infer T1, infer R> ? PromisedFunc2<T1, R> :
  T[K] extends Func1<infer R> ? PromisedFunc1<R> :
  T[K] extends Func<infer R> ? PromisedFunc<R> :
  () => Promise<T[K]>;
};

Copy link
Member Author

Choose a reason for hiding this comment

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

As TS 3 is out since a couple of hours, I've implemented this in the latest commit. Awesome!

};

const BROKEN_PIPE_ERROR_CODE = 'EPIPE';
const IPC_CHANNEL_CLOSED_ERROR_CODE = 'ERR_IPC_CHANNEL_CLOSED';
const TIMEOUT_FOR_DISPOSE = 2000;
const MESSAGE_QUEUE_SIZE = 20;

export default class ChildProcessProxy<T> {
readonly proxy: ChildProxy<T> = {} as ChildProxy<T>;
readonly proxy: Promisified<T>;

private worker: ChildProcess;
private initTask: Task;
private disposeTask: Task<void>;
private disposeTask: ExpirableTask<void> | undefined;
private currentError: ChildProcessCrashedError | undefined;
private workerTasks: Task<any>[] = [];
private log = getLogger(ChildProcessProxy.name);
private recentMessagesQueue: string[] = [];
private isDisposed = false;

private constructor(requirePath: string, loggingContext: LoggingClientContext, plugins: string[], private constructorFunction: { new(...params: any[]): T }, constructorParams: any[]) {
this.worker = fork(require.resolve('./ChildProcessProxyWorker'), [autoStart], { silent: false, execArgv: [] });
private constructor(requirePath: string, loggingContext: LoggingClientContext, plugins: string[], workingDirectory: string, constructorParams: any[]) {
this.worker = fork(require.resolve('./ChildProcessProxyWorker'), [autoStart], { silent: true, execArgv: [] });
this.initTask = new Task();
this.log.debug('Starting %s in a child process', requirePath);
this.send({
kind: WorkerMessageKind.Init,
loggingContext,
plugins,
requirePath,
constructorArgs: constructorParams
constructorArgs: constructorParams,
workingDirectory
});
this.listenToWorkerMessages();
this.initProxy();
this.listenForMessages();
this.listenToStdoutAndStderr();
// This is important! Be sure to bind to `this`
this.handleUnexpectedExit = this.handleUnexpectedExit.bind(this);
this.handleError = this.handleError.bind(this);
this.worker.on('exit', this.handleUnexpectedExit);
Copy link
Member

Choose a reason for hiding this comment

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

Can we save the exit and error worker messages on the worker type itself so we don't have to use magic strings? Or are they a part of NodeJS?

Copy link
Member Author

@nicojs nicojs Jul 29, 2018

Choose a reason for hiding this comment

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

What do you mean? The worker here is an instance of ChildProcess right?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean NodeJS's ChildPRocess? Do we choose to name the event 'exit' or does NodeJS determine this name? If NodeJS determines it, this code is fine. If we personally have code that raises an event with name 'exit' then we should use a constant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a node thing. It works based on events. Not too great if you ask me, but that's how it works.

this.worker.on('error', this.handleError);
this.proxy = this.initProxy();
}

/**
* Creates a proxy where each function of the object created using the constructorFunction arg is ran inside of a child process
*/
static create<T, P1>(requirePath: string, loggingContext: LoggingClientContext, plugins: string[], constructorFunction: { new(arg: P1): T }, arg: P1): ChildProcessProxy<T>;
static create<T, P1>(requirePath: string, loggingContext: LoggingClientContext, plugins: string[], workingDirectory: string, constructorFunction: { new(arg: P1): T }, arg: P1): ChildProcessProxy<T>;
/**
* Creates a proxy where each function of the object created using the constructorFunction arg is ran inside of a child process
*/
static create<T, P1, P2>(requirePath: string, loggingContext: LoggingClientContext, plugins: string[], constructorFunction: { new(arg: P1, arg2: P2): T }, arg1: P1, arg2: P2): ChildProcessProxy<T>;
static create<T, P1, P2>(requirePath: string, loggingContext: LoggingClientContext, plugins: string[], workingDirectory: string, constructorFunction: { new(arg: P1, arg2: P2): T }, arg1: P1, arg2: P2): ChildProcessProxy<T>;
/**
* Creates a proxy where each function of the object created using the constructorFunction arg is ran inside of a child process
*/
static create<T>(requirePath: string, loggingContext: LoggingClientContext, plugins: string[], constructorFunction: { new(...params: any[]): T }, ...constructorArgs: any[]) {
return new ChildProcessProxy(requirePath, loggingContext, plugins, constructorFunction, constructorArgs);
static create<T>(requirePath: string, loggingContext: LoggingClientContext, plugins: string[], workingDirectory: string, _: { new(...params: any[]): T }, ...constructorArgs: any[]) {
return new ChildProcessProxy(requirePath, loggingContext, plugins, workingDirectory, constructorArgs);
}

private send(message: WorkerMessage) {
this.worker.send(serialize(message));
}

private initProxy() {
Object.keys(this.constructorFunction.prototype).forEach(methodName => {
this.proxyMethod(methodName as keyof T);
private initProxy(): Promisified<T> {
// This proxy is a genuine javascript `Proxy` class
// More info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy
const self = this;
return new Proxy({} as Promisified<T>, {
get(_, propertyKey) {
if (typeof propertyKey === 'string') {
return self.forward(propertyKey);
} else {
return undefined;
}
}
});
}

private proxyMethod(methodName: any) {
this.proxy[(methodName as keyof T)] = (...args: any[]) => {
const workerTask = new Task<any>();
this.initTask.promise.then(() => {
private forward(methodName: string) {
return (...args: any[]) => {
if (this.currentError) {
return Promise.reject(this.currentError);
} else {
const workerTask = new Task<any>();
const correlationId = this.workerTasks.push(workerTask) - 1;
this.send({
kind: WorkerMessageKind.Work,
correlationId,
methodName,
args
this.initTask.promise.then(() => {
this.send({
kind: WorkerMessageKind.Call,
correlationId,
methodName,
args
});
});
});
return workerTask.promise;
return workerTask.promise;
}
};
}

private listenToWorkerMessages() {
private listenForMessages() {
this.worker.on('message', (serializedMessage: string) => {
const message: ParentMessage = deserialize(serializedMessage, [File]);
switch (message.kind) {
Expand All @@ -83,12 +117,16 @@ export default class ChildProcessProxy<T> {
break;
case ParentMessageKind.Result:
this.workerTasks[message.correlationId].resolve(message.result);
delete this.workerTasks[message.correlationId];
break;
case ParentMessageKind.Rejection:
this.workerTasks[message.correlationId].reject(new Error(message.error));
delete this.workerTasks[message.correlationId];
break;
case ParentMessageKind.DisposeCompleted:
this.disposeTask.resolve(undefined);
if (this.disposeTask) {
this.disposeTask.resolve(undefined);
}
break;
default:
this.logUnidentifiedMessage(message);
Expand All @@ -97,12 +135,93 @@ export default class ChildProcessProxy<T> {
});
}

private listenToStdoutAndStderr() {
const traceEnabled = this.log.isTraceEnabled();
Copy link
Member

Choose a reason for hiding this comment

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

Will this value ever change during execution? If not, I would suggest just reading the value on the line if (traceEnabled)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Will change that

const handleData = (data: Buffer) => {
const message = data.toString();
this.recentMessagesQueue.push(message);
if (this.recentMessagesQueue.length > MESSAGE_QUEUE_SIZE) {
this.recentMessagesQueue.shift();
}

if (traceEnabled) {
this.log.trace(message);
}
};

if (this.worker.stdout) {
this.worker.stdout.on('data', handleData);
}

if (this.worker.stderr) {
this.worker.stderr.on('data', handleData);
}
}

private reportError(error: Error) {
this.workerTasks
.filter(task => !task.isResolved)
.forEach(task => task.reject(error));
}

private handleUnexpectedExit(code: number, signal: string) {
this.isDisposed = true;
const output = this.recentMessagesQueue.join(os.EOL);

if (processOutOfMemory()) {
this.currentError = new OutOfMemoryError(this.worker.pid, code);
this.log.warn(`Child process [pid ${this.currentError.pid}] ran out of memory. Stdout and stderr are logged on debug level.`);
this.log.debug(stdoutAndStderr());
} else {
this.currentError = new ChildProcessCrashedError(this.worker.pid, code);
this.log.warn(`Child process [pid ${this.worker.pid}] exited unexpectedly with exit code ${code} (${signal || 'without signal'}). ${stdoutAndStderr()}`, this.currentError);
}

this.reportError(this.currentError);

function processOutOfMemory() {
return output.indexOf('JavaScript heap out of memory') >= 0;
}

function stdoutAndStderr() {
if (output.length) {
return `Last part of stdout and stderr was:${os.EOL}${padLeft(output)}`;
} else {
return 'Stdout and stderr were empty.';
}
}
}

private handleError(error: Error) {
if (this.innerProcessIsCrashed(error)) {
this.log.warn(`Child process [pid ${this.worker.pid}] has crashed. See other warning messages for more info.`, error);
this.reportError(new ChildProcessCrashedError(this.worker.pid, undefined, undefined, error));
} else {
this.reportError(error);
}
}

private innerProcessIsCrashed(error: any) {
return isErrnoException(error) && (error.code === BROKEN_PIPE_ERROR_CODE || error.code === IPC_CHANNEL_CLOSED_ERROR_CODE);
}

public dispose(): Promise<void> {
this.disposeTask = new Task();
this.send({ kind: WorkerMessageKind.Dispose });
return this.disposeTask.promise
.then(() => this.worker.kill())
.catch(() => this.worker.kill());
this.worker.removeListener('exit', this.handleUnexpectedExit);
if (this.isDisposed) {
return Promise.resolve();
} else {
this.log.debug('Disposing of worker process %s', this.worker.pid);
const killWorker = () => {
this.log.debug('Kill %s', this.worker.pid);
kill(this.worker.pid);
this.isDisposed = true;
};
this.disposeTask = new ExpirableTask(TIMEOUT_FOR_DISPOSE);
this.send({ kind: WorkerMessageKind.Dispose });
return this.disposeTask.promise
.then(killWorker)
.catch(killWorker);
}
}

private logUnidentifiedMessage(message: never) {
Expand Down
Loading