From d931d52796c64254d07bac98d2a9ee2884816b12 Mon Sep 17 00:00:00 2001 From: John Kane Date: Thu, 21 Jul 2022 11:26:05 +0100 Subject: [PATCH] fix: don't restart on bad hardhat config (#212) To stop a restart loop on bad hardhat config the hardhat worker has been enhanced to have a lifecycle. The background process starts as UNINITIALIZED. On `init` it moves to `STARTING`. Any unexpected exits during starting moves it to `INITIALIZATION_ERRORED`. Only an edit to the config file will trigger a retry from this state. On initialization completing within the background process, it will send a `INITIALISATION_COMPLETE` message, that will move the state to `RUNNING`. An unexpected exit while `RUNNING` will lead to a restart. Validation requests to the process if it is not in the running state (so errored or still starting), leads to a validator error which will be displayed to the user. In response to #211. --- .../src/services/validation/HardhatWorker.ts | 245 +++++++++++++++--- .../validation/compilerProcessFactory.ts | 4 +- server/src/services/validation/validate.ts | 44 +++- server/src/services/validation/worker.ts | 11 +- .../worker/initialiseWorkerState.ts | 7 +- server/src/types.ts | 41 ++- .../test/services/validation/hardhatWorker.ts | 154 +++++++++++ server/test/services/validation/validation.ts | 68 ++++- 8 files changed, 519 insertions(+), 55 deletions(-) create mode 100644 server/test/services/validation/hardhatWorker.ts diff --git a/server/src/services/validation/HardhatWorker.ts b/server/src/services/validation/HardhatWorker.ts index 8d5d3a2f..108baf7c 100644 --- a/server/src/services/validation/HardhatWorker.ts +++ b/server/src/services/validation/HardhatWorker.ts @@ -1,70 +1,119 @@ -/* istanbul ignore file: wrapper around process */ -import * as path from "path"; import * as childProcess from "child_process"; +import * as path from "path"; import { HardhatProject } from "@analyzer/HardhatProject"; import { Logger } from "@utils/Logger"; import { + InitialisationCompleteMessage, InvalidatePreprocessingCacheMessage, ValidateCommand, ValidationCompleteMessage, WorkerProcess, } from "../../types"; +const UNINITIALIZED = "UNINITIALIZED"; +const STARTING = "STARTING"; +const RUNNING = "RUNNING"; +const INITIALIZATION_ERRORED = "INITIALIZATION_ERRORED"; + +type HardhatWorkerStatus = + | typeof UNINITIALIZED + | typeof INITIALIZATION_ERRORED + | typeof STARTING + | typeof RUNNING; + +export function createProcessFor( + project: HardhatProject +): childProcess.ChildProcess { + return childProcess.fork(path.resolve(__dirname, "worker.js"), { + cwd: project.basePath, + detached: true, + }); +} + export class HardhatWorker implements WorkerProcess { public project: HardhatProject; - private child: childProcess.ChildProcess | null; - private logger: Logger; - private jobCount: number; - - private jobs: { + public status: HardhatWorkerStatus; + public jobs: { [key: string]: { resolve: (message: ValidationCompleteMessage) => void; reject: (err: string) => void; }; }; - constructor(project: HardhatProject, logger: Logger) { - this.project = project; + private child: childProcess.ChildProcess | null; + private createProcessFor: ( + project: HardhatProject + ) => childProcess.ChildProcess; + private logger: Logger; + private jobCount: number; + + constructor( + project: HardhatProject, + givenCreateProcessFor: ( + project: HardhatProject + ) => childProcess.ChildProcess, + logger: Logger + ) { this.child = null; this.jobCount = 0; this.jobs = {}; + + this.project = project; + this.createProcessFor = givenCreateProcessFor; this.logger = logger; + + this.status = UNINITIALIZED; } + /** + * Setup the background validation process along with listeners + * on the LSP side. + * + * The status immediately moves from UNINITIALIZED -> STARTING. An + * `INITIALISATION_COMPLETE` message from the process will move + * the status to RUNNING (an unexpected exit will move it to + * INITIALIZATION_ERRORED). + */ public init() { - this.child = childProcess.fork(path.resolve(__dirname, "worker.js"), { - cwd: this.project.basePath, - detached: true, - }); - - this.child.on("message", (message: ValidationCompleteMessage) => { - if (!(message.jobId in this.jobs)) { - this.logger.error("No job registered for validation complete"); - return; - } + if (![UNINITIALIZED, INITIALIZATION_ERRORED].includes(this.status)) { + throw new Error("Cannot start a worker thread that has already started"); + } - const { resolve } = this.jobs[message.jobId]; + this.status = STARTING; - delete this.jobs[message.jobId]; + this.child = this.createProcessFor(this.project); - resolve(message); - }); + // deal with messages sent from the background process to the LSP + this.child.on( + "message", + (message: InitialisationCompleteMessage | ValidationCompleteMessage) => { + switch (message.type) { + case "INITIALISATION_COMPLETE": + this.status = RUNNING; + this.logger.trace( + `initialisation complete for ${this.project.basePath}` + ); + break; + case "VALIDATION_COMPLETE": + this._validationComplete(message); + break; + default: + this._unexectpedMessage(message); + break; + } + } + ); + // errors on the background thread are logged this.child.on("error", (err) => { this.logger.error(err); }); - this.child.on("exit", (code, signal) => { - this.logger.trace( - `Hardhat Worker Process restart (${code}): ${this.project.basePath}` - ); - - if (code === 0 || signal !== null) { - return; - } - - return this.restart(); - }); + // if the background process exits due to an error + // we restart if it has previously been running, + // if exits during initialization, we leave it in + // the errored state + this.child.on("exit", this.handleExit.bind(this)); } public async validate({ @@ -88,6 +137,14 @@ export class HardhatWorker implements WorkerProcess { return reject(new Error("No child process to send validation")); } + if (this.status !== RUNNING) { + return this._validationBlocked( + { jobId, projectBasePath }, + resolve, + reject + ); + } + this.jobs[jobId] = { resolve, reject }; const message: ValidateCommand = { @@ -108,6 +165,12 @@ export class HardhatWorker implements WorkerProcess { }); } + /** + * Inform the background validation process to clear its file caches + * and reread the solidity files from disk on the next job. + * + * @returns whether the cace was cleared + */ public async invalidatePreprocessingCache(): Promise { return new Promise((resolve, reject) => { if (this.child === null) { @@ -116,6 +179,11 @@ export class HardhatWorker implements WorkerProcess { ); } + // Only running validators can have their cache cleared + if (this.status !== RUNNING) { + return resolve(false); + } + const message: InvalidatePreprocessingCacheMessage = { type: "INVALIDATE_PREPROCESSING_CACHE", }; @@ -130,19 +198,122 @@ export class HardhatWorker implements WorkerProcess { }); } + /** + * Stop the current background validation process. + * + * The status will be set back to the unstarted state (UNINITIALIZED). + */ public kill() { this.child?.kill(); + // reset status to allow restarting in future + this.status = UNINITIALIZED; } + /** + * Stop the current background validation process and start a new one. + * + * The jobs being currently processeded are all cancelled. + */ public async restart(): Promise { + this.logger.trace(`Restarting hardhat worker for ${this.project.basePath}`); + + this._cancelCurrentJobs(); + + this.kill(); + this.init(); + } + + public handleExit(code: number | null, signal: NodeJS.Signals | null) { + this.logger.trace( + `Hardhat Worker Process restart (${code}): ${this.project.basePath}` + ); + + if (code === 0 || signal !== null) { + this.status = UNINITIALIZED; + this._cancelCurrentJobs(); + return; + } + + if (this.status === STARTING) { + this.status = INITIALIZATION_ERRORED; + this._cancelCurrentJobs(); + return; + } + + if (this.status !== RUNNING) { + this.logger.error( + new Error( + "Exit from validator that is already UNINITIALIZED/INITIALIZATION_ERRORED" + ) + ); + return; + } + + return this.restart(); + } + + private _validationComplete(message: ValidationCompleteMessage) { + if (!(message.jobId in this.jobs)) { + this.logger.error("No job registered for validation complete"); + return; + } + + const { resolve } = this.jobs[message.jobId]; + + delete this.jobs[message.jobId]; + + resolve(message); + } + + private _unexectpedMessage(message: never) { + this.logger.error(new Error(`Unexpected error type: ${message}`)); + } + + private _validationBlocked( + { jobId, projectBasePath }: { jobId: number; projectBasePath: string }, + resolve: ( + value: ValidationCompleteMessage | PromiseLike + ) => void, + _reject: (reason?: string) => void + ): void { + if (this.status === STARTING) { + return resolve({ + type: "VALIDATION_COMPLETE", + status: "VALIDATOR_ERROR", + jobId, + projectBasePath, + reason: "validator-starting", + }); + } + + if (this.status === "INITIALIZATION_ERRORED") { + return resolve({ + type: "VALIDATION_COMPLETE", + status: "VALIDATOR_ERROR", + jobId, + projectBasePath, + reason: "validator-initialization-failed", + }); + } + + return resolve({ + type: "VALIDATION_COMPLETE", + status: "VALIDATOR_ERROR", + jobId, + projectBasePath, + reason: "validator-in-unexpected-state", + }); + } + + /** + * Reject any open jobs whose result is still promised. + */ + private _cancelCurrentJobs() { for (const jobId of Object.keys(this.jobs)) { const { reject } = this.jobs[jobId]; reject("Worker process restarted"); delete this.jobs[jobId]; } - - this.kill(); - this.init(); } } diff --git a/server/src/services/validation/compilerProcessFactory.ts b/server/src/services/validation/compilerProcessFactory.ts index b512bf90..b759d4ae 100644 --- a/server/src/services/validation/compilerProcessFactory.ts +++ b/server/src/services/validation/compilerProcessFactory.ts @@ -2,11 +2,11 @@ import { HardhatProject } from "@analyzer/HardhatProject"; import { Logger } from "@utils/Logger"; import { WorkerProcess } from "../../types"; -import { HardhatWorker } from "./HardhatWorker"; +import { createProcessFor, HardhatWorker } from "./HardhatWorker"; export function compilerProcessFactory( project: HardhatProject, logger: Logger ): WorkerProcess { - return new HardhatWorker(project, logger); + return new HardhatWorker(project, createProcessFor, logger); } diff --git a/server/src/services/validation/validate.ts b/server/src/services/validation/validate.ts index 53a05a6d..4f77b6c0 100644 --- a/server/src/services/validation/validate.ts +++ b/server/src/services/validation/validate.ts @@ -15,6 +15,7 @@ import { ValidationJobFailureNotification, ValidationJobStatusNotification, ValidationPass, + ValidatorError, WorkerProcess, } from "../../types"; import { getOpenDocumentsInProject } from "../../queries/getOpenDocumentsInProject"; @@ -97,6 +98,8 @@ function sendResults( return hardhatThrownFail(serverState, change, completeMessage); case "JOB_COMPLETION_ERROR": return jobCompletionErrorFail(serverState, change, completeMessage); + case "VALIDATOR_ERROR": + return validatorErrorFail(serverState, change, completeMessage); case "UNKNOWN_ERROR": return unknownErrorFail(serverState, change, completeMessage); case "VALIDATION_FAIL": @@ -176,6 +179,21 @@ function jobCompletionErrorFail( serverState.connection.sendNotification("custom/validation-job-status", data); } +function validatorErrorFail( + serverState: ServerState, + { document }: TextDocumentChangeEvent, + validatorError: ValidatorError +) { + serverState.connection.sendDiagnostics({ + uri: document.uri, + diagnostics: [], + }); + + const data: ValidationJobStatusNotification = jobStatusFrom(validatorError); + + serverState.connection.sendNotification("custom/validation-job-status", data); +} + function unknownErrorFail( serverState: ServerState, { document }: TextDocumentChangeEvent, @@ -219,7 +237,10 @@ function sendValidationProcessSuccess( function jobStatusFrom({ projectBasePath, reason, -}: JobCompletionError): ValidationJobFailureNotification { +}: { + projectBasePath: string; + reason: string; +}): ValidationJobFailureNotification { switch (reason) { case "directly-imports-incompatible-file": return { @@ -249,6 +270,27 @@ function jobStatusFrom({ reason, displayText: "no compatibile solc version found", }; + case "validator-starting": + return { + validationRun: false, + projectBasePath, + reason, + displayText: "validator starting", + }; + case "validator-initialization-failed": + return { + validationRun: false, + projectBasePath, + reason, + displayText: "unable to load hardhat config", + }; + case "validator-in-unexpected-state": + return { + validationRun: false, + projectBasePath, + reason, + displayText: "validator in unexpected state", + }; default: return { validationRun: false, diff --git a/server/src/services/validation/worker.ts b/server/src/services/validation/worker.ts index 0b134de3..ddb71e05 100644 --- a/server/src/services/validation/worker.ts +++ b/server/src/services/validation/worker.ts @@ -1,5 +1,8 @@ /* istanbul ignore file: setup point for validation process */ -import type { ValidationCompleteMessage } from "../../types"; +import type { + InitialisationCompleteMessage, + ValidationCompleteMessage, +} from "../../types"; import { dispatch } from "./worker/dispatch"; import { initialiseWorkerState } from "./worker/initialiseWorkerState"; import { setupWorkerLogger } from "./worker/setupWorkerLogger"; @@ -13,9 +16,13 @@ const initialiseWorker = async () => { workerLogger.trace("[WORKER] Waiting for messages ..."); process.on("message", dispatch(workserState)); + + await workserState.send({ type: "INITIALISATION_COMPLETE" }); }; -function send(message: ValidationCompleteMessage): Promise { +function send( + message: InitialisationCompleteMessage | ValidationCompleteMessage +): Promise { return new Promise((resolve, reject) => { if (!process.send) { return; diff --git a/server/src/services/validation/worker/initialiseWorkerState.ts b/server/src/services/validation/worker/initialiseWorkerState.ts index 610342ec..e918a0c5 100644 --- a/server/src/services/validation/worker/initialiseWorkerState.ts +++ b/server/src/services/validation/worker/initialiseWorkerState.ts @@ -1,13 +1,16 @@ /* istanbul ignore file: top level node loading */ import path from "path"; import type { + InitialisationCompleteMessage, ValidationCompleteMessage, WorkerLogger, WorkerState, } from "../../../types"; export async function initialiseWorkerState( - send: (message: ValidationCompleteMessage) => Promise, + send: ( + message: InitialisationCompleteMessage | ValidationCompleteMessage + ) => Promise, logger: WorkerLogger ): Promise { let hre; @@ -25,7 +28,7 @@ export async function initialiseWorkerState( hre = require(`${hardhatBase}/internal/lib/hardhat-lib.js`); } catch (err) { - throw new Error("Unable to initialize Hardhat Runtime Environment"); + throw new Error(`Unable to initialize Hardhat Runtime Environment`); } const { diff --git a/server/src/types.ts b/server/src/types.ts index af0865be..b861cc31 100644 --- a/server/src/types.ts +++ b/server/src/types.ts @@ -163,7 +163,9 @@ export interface WorkerState { // eslint-disable-next-line @typescript-eslint/naming-convention TASK_COMPILE_SOLIDITY_READ_FILE: string; }; - send: (message: ValidationCompleteMessage) => Promise; + send: ( + message: InitialisationCompleteMessage | ValidationCompleteMessage + ) => Promise; logger: WorkerLogger; } @@ -227,6 +229,10 @@ export interface HardhatCompilerError { type: "DeclarationError"; } +/** + * While running the validation job, an error was thrown + * from within hardhat. + */ export interface HardhatThrownError { type: "VALIDATION_COMPLETE"; status: "HARDHAT_ERROR"; @@ -235,6 +241,23 @@ export interface HardhatThrownError { hardhatError: HardhatError; } +/** + * An error with the background validation thread + * e.g. failed to start or has died. + */ +export interface ValidatorError { + type: "VALIDATION_COMPLETE"; + status: "VALIDATOR_ERROR"; + jobId: number; + projectBasePath: string; + reason: string; +} + +/** + * An error in completing the validation job + * e.g. prerequisutes of compile failed, downloading + * the solc compiler etc. + */ export interface JobCompletionError { type: "VALIDATION_COMPLETE"; status: "JOB_COMPLETION_ERROR"; @@ -251,6 +274,9 @@ export interface UnknownError { error: unknown; } +/** + * The validation job ran and solc returned warnings/errors + */ export interface ValidationFail { type: "VALIDATION_COMPLETE"; status: "VALIDATION_FAIL"; @@ -260,6 +286,10 @@ export interface ValidationFail { errors: HardhatCompilerError[]; } +/** + * The validation job ran and solc return no warnings/errors + * indicating the code would compile. + */ export interface ValidationPass { type: "VALIDATION_COMPLETE"; status: "VALIDATION_PASS"; @@ -269,6 +299,10 @@ export interface ValidationPass { sources: string[]; } +/** + * The validation job was cancelled part way through, + * probably because a new edit came in. + */ export interface CancelledValidation { type: "VALIDATION_COMPLETE"; status: "CANCELLED"; @@ -276,11 +310,16 @@ export interface CancelledValidation { projectBasePath: string; } +export interface InitialisationCompleteMessage { + type: "INITIALISATION_COMPLETE"; +} + export type ValidationCompleteMessage = | ValidationPass | ValidationFail | HardhatThrownError | JobCompletionError + | ValidatorError | CancelledValidation | UnknownError; diff --git a/server/test/services/validation/hardhatWorker.ts b/server/test/services/validation/hardhatWorker.ts new file mode 100644 index 00000000..6a2621b0 --- /dev/null +++ b/server/test/services/validation/hardhatWorker.ts @@ -0,0 +1,154 @@ +import { HardhatProject } from "@analyzer/HardhatProject"; +import { HardhatWorker } from "@services/validation/HardhatWorker"; +import { assert } from "chai"; +import * as sinon from "sinon"; +import { setupMockLogger } from "../../helpers/setupMockLogger"; + +describe("Hardhat Worker", () => { + const exampleProj: HardhatProject = { + type: "hardhat", + basePath: "/example", + configPath: "/example/hardhat.config.js", + workspaceFolder: { + name: "example", + uri: "/example", + }, + }; + + describe("initialization", () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let mockProcessCreator: any; + let hardhatWorker: HardhatWorker; + + beforeEach(() => { + const mockLogger = setupMockLogger(); + mockProcessCreator = function () { + return { + on: sinon.spy(), + }; + }; + + hardhatWorker = new HardhatWorker( + exampleProj, + mockProcessCreator, + mockLogger + ); + }); + + it("should set the worker to STARTING", () => { + hardhatWorker.init(); + + assert.equal(hardhatWorker.status, "STARTING"); + }); + + describe("when already starting", () => { + it("should error", () => { + hardhatWorker.status = "STARTING"; + + assert.throws( + () => hardhatWorker.init(), + "Cannot start a worker thread that has already started" + ); + }); + }); + + describe("when already running", () => { + it("should error", () => { + hardhatWorker.status = "RUNNING"; + + assert.throws( + () => hardhatWorker.init(), + "Cannot start a worker thread that has already started" + ); + }); + }); + }); + + describe("on exit", () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let mockChildProcess: any; + let hardhatWorker: HardhatWorker; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let openJob: any; + + beforeEach(() => { + const mockLogger = setupMockLogger(); + mockChildProcess = { + kill: sinon.spy(), + on: sinon.spy(), + }; + + hardhatWorker = new HardhatWorker( + exampleProj, + () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return mockChildProcess as any; + }, + mockLogger + ); + + openJob = { + resolve: sinon.spy(), + reject: sinon.spy(), + }; + + hardhatWorker.jobs.example = openJob; + + hardhatWorker.init(); + }); + + describe("when running", () => { + beforeEach(() => { + hardhatWorker.status = "RUNNING"; + + hardhatWorker.handleExit(1, null); + }); + + it("should cancel any open jobs", () => { + assert.lengthOf(Object.values(hardhatWorker.jobs), 0); + sinon.assert.called(openJob.reject); + }); + + it("should restart", () => { + sinon.assert.called(mockChildProcess.kill); + }); + + it("should set the worker to STARTING", () => { + assert.equal(hardhatWorker.status, "STARTING"); + }); + }); + + describe("when starting", () => { + beforeEach(() => { + hardhatWorker.handleExit(1, null); + }); + + it("should not restart", () => { + sinon.assert.notCalled(mockChildProcess.kill); + }); + + it("should set the worker to INITIALIZATION_ERRORED", () => { + assert.equal(hardhatWorker.status, "INITIALIZATION_ERRORED"); + }); + }); + + describe("termination through signal", () => { + beforeEach(() => { + hardhatWorker.handleExit(1, "SIGTERM"); + }); + + it("should cancel any open jobs", () => { + assert.lengthOf(Object.values(hardhatWorker.jobs), 0); + sinon.assert.called(openJob.reject); + }); + + it("should not restart", () => { + sinon.assert.notCalled(mockChildProcess.kill); + }); + + it("should set the worker back to UNINITIALIZED", () => { + assert.equal(hardhatWorker.status, "UNINITIALIZED"); + }); + }); + }); +}); diff --git a/server/test/services/validation/validation.ts b/server/test/services/validation/validation.ts index 73180456..2d0abb62 100644 --- a/server/test/services/validation/validation.ts +++ b/server/test/services/validation/validation.ts @@ -13,6 +13,7 @@ import { UnknownError, ValidationCompleteMessage, ValidationJobStatusNotification, + ValidatorError, } from "../../../src/types"; import { forceToUnixStyle } from "../../helpers/forceToUnixStyle"; import { prependWithSlash } from "../../helpers/prependWithSlash"; @@ -651,6 +652,29 @@ describe("Parser", () => { }); }); + describe("validator error", () => { + describe("initialization is still in progress", () => { + it("sends a failure status message", async () => + assertValidatorError("validator-starting", "validator starting")); + }); + + describe("initialization failed", () => { + it("sends a failure status message", async () => + assertValidatorError( + "validator-initialization-failed", + "unable to load hardhat config" + )); + }); + + describe("initialization failed", () => { + it("sends a failure status message", async () => + assertValidatorError( + "validator-in-unexpected-state", + "validator in unexpected state" + )); + }); + }); + describe("job completion error", () => { describe("directly imports incompatible file", () => { it("sends a failure status message", async () => @@ -1036,19 +1060,43 @@ async function validateReturningWorkerMessage( async function assertJobCompletionError( reason: string, expectedDisplayText: string +) { + return assertError( + { + type: "VALIDATION_COMPLETE", + status: "JOB_COMPLETION_ERROR", + jobId: 1, + projectBasePath: "/projects/example", + reason, + }, + { reason, displayText: expectedDisplayText } + ); +} + +async function assertValidatorError( + reason: string, + expectedDisplayText: string +) { + return assertError( + { + type: "VALIDATION_COMPLETE", + status: "VALIDATOR_ERROR", + jobId: 1, + projectBasePath: "/projects/example", + reason, + }, + { reason, displayText: expectedDisplayText } + ); +} + +async function assertError( + workerReturnMessage: JobCompletionError | ValidatorError, + expected: { reason: string; displayText: string } ) { const sendDiagnostics = sinon.spy(); const sendNotification = sinon.spy(); const logger = setupMockLogger(); - const workerReturnMessage: JobCompletionError = { - type: "VALIDATION_COMPLETE", - status: "JOB_COMPLETION_ERROR", - jobId: 1, - projectBasePath: "/projects/example", - reason, - }; - await validateReturningWorkerMessage(workerReturnMessage, { sendDiagnosticsSpy: sendDiagnostics, sendNotificationSpy: sendNotification, @@ -1067,8 +1115,8 @@ async function assertJobCompletionError( const expectedFailureStatus: ValidationJobStatusNotification = { validationRun: false, projectBasePath: "/projects/example", - reason, - displayText: expectedDisplayText, + reason: expected.reason, + displayText: expected.displayText, }; assert.deepStrictEqual(sendNotification.args[0][1], expectedFailureStatus);