From f0b41048ad65ae63496af06f1b62c3faa5911c54 Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Mon, 24 Sep 2018 15:48:14 +0200 Subject: [PATCH] fix: handle all promises and add no-floating-promises linting rule This addresses a few bugs, including error handling issues in the TaskGraph. --- garden-cli/package-lock.json | 11 ++ garden-cli/package.json | 2 + garden-cli/src/commands/logs.ts | 2 +- .../logger/writers/fancy-terminal-writer.ts | 7 +- garden-cli/src/plugins/kubernetes/actions.ts | 2 +- garden-cli/src/plugins/kubernetes/helm.ts | 2 +- garden-cli/src/process.ts | 4 +- garden-cli/src/task-graph.ts | 117 +++--------------- garden-cli/src/vcs/git.ts | 12 -- garden-cli/src/watch.ts | 2 +- tslint.json | 1 + 11 files changed, 40 insertions(+), 122 deletions(-) diff --git a/garden-cli/package-lock.json b/garden-cli/package-lock.json index 13cbea06fd..1a328c3be3 100644 --- a/garden-cli/package-lock.json +++ b/garden-cli/package-lock.json @@ -633,6 +633,12 @@ "integrity": "sha512-NWKCFU+yFaTY4yY1qNiAnlb085k2ZUKbgJ/ViZka13T90uQ7e17htntVOE5y2RcnTpoHvjMp4hyhZoU0mDdSlA==", "dev": true }, + "@types/p-queue": { + "version": "2.3.1", + "resolved": "https://registry.npmjs.org/@types/p-queue/-/p-queue-2.3.1.tgz", + "integrity": "sha512-JyO7uMAtkcMMULmsTQ4t/lCC8nxirTtweGG1xAFNNIAoC1RemmeIxq8PiKghuEy99XdbS6Lwx4zpbXUjfeSSAA==", + "dev": true + }, "@types/path-is-inside": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/@types/path-is-inside/-/path-is-inside-1.0.0.tgz", @@ -8561,6 +8567,11 @@ "p-limit": "^1.1.0" } }, + "p-queue": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/p-queue/-/p-queue-3.0.0.tgz", + "integrity": "sha512-2tv/MRmPXfmfnjLLJAHl+DdU8p2DhZafAnlpm/C/T5BpF5L9wKz5tMin4A4N2zVpJL2YMhPlRmtO7s5EtNrjfA==" + }, "p-try": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/p-try/-/p-try-1.0.0.tgz", diff --git a/garden-cli/package.json b/garden-cli/package.json index 7e4fa33bf8..14dee2c127 100644 --- a/garden-cli/package.json +++ b/garden-cli/package.json @@ -58,6 +58,7 @@ "node-emoji": "^1.8.1", "node-pty": "^0.7.6", "normalize-url": "^3.2.0", + "p-queue": "^3.0.0", "path-is-inside": "^1.0.2", "shx": "^0.3.2", "snyk": "^1.90.2", @@ -100,6 +101,7 @@ "@types/node": "^10.7.0", "@types/node-emoji": "^1.8.0", "@types/normalize-url": "^1.9.1", + "@types/p-queue": "^2.3.1", "@types/path-is-inside": "^1.0.0", "@types/prettyjson": "0.0.28", "@types/string-width": "^2.0.0", diff --git a/garden-cli/src/commands/logs.ts b/garden-cli/src/commands/logs.ts index e5196f3b5b..a02a73100d 100644 --- a/garden-cli/src/commands/logs.ts +++ b/garden-cli/src/commands/logs.ts @@ -63,7 +63,7 @@ export class LogsCommand extends Command { const stream = new Stream() // TODO: use basic logger (no need for fancy stuff here, just causes flickering) - stream.forEach((entry) => { + void stream.forEach((entry) => { // TODO: color each service differently for easier visual parsing let timestamp = " " diff --git a/garden-cli/src/logger/writers/fancy-terminal-writer.ts b/garden-cli/src/logger/writers/fancy-terminal-writer.ts index 65f1419ef9..922b8074a4 100644 --- a/garden-cli/src/logger/writers/fancy-terminal-writer.ts +++ b/garden-cli/src/logger/writers/fancy-terminal-writer.ts @@ -20,7 +20,6 @@ import { import { LogEntry } from "../log-entry" import { Logger } from "../logger" import { LogLevel } from "../log-node" -import { sleep } from "../../util/util" import { getChildEntries, getTerminalWidth, interceptStream, validate } from "../util" import { Writer, WriterConfig } from "./base" @@ -149,13 +148,11 @@ export class FancyTerminalWriter extends Writer { this.updatePending = true // Resume processing if idle and original update is still pending - const maybeResume = async () => { - await sleep(FANCY_LOGGER_THROTTLE_MS) + setTimeout(() => { if (this.updatePending) { this.handleGraphChange(logEntry, logger, true) } - } - maybeResume() + }, FANCY_LOGGER_THROTTLE_MS) return } } diff --git a/garden-cli/src/plugins/kubernetes/actions.ts b/garden-cli/src/plugins/kubernetes/actions.ts index 65a2b3eac3..cdaf748ef5 100644 --- a/garden-cli/src/plugins/kubernetes/actions.ts +++ b/garden-cli/src/plugins/kubernetes/actions.ts @@ -290,7 +290,7 @@ export async function getServiceLogs( try { timestamp = moment(timestampStr).toDate() } catch { } - stream.write({ serviceName: service.name, timestamp, msg }) + void stream.write({ serviceName: service.name, timestamp, msg }) }) proc.stderr.pipe(process.stderr) diff --git a/garden-cli/src/plugins/kubernetes/helm.ts b/garden-cli/src/plugins/kubernetes/helm.ts index f48043ed86..8aebf61865 100644 --- a/garden-cli/src/plugins/kubernetes/helm.ts +++ b/garden-cli/src/plugins/kubernetes/helm.ts @@ -208,7 +208,7 @@ async function build({ ctx, module, logEntry }: BuildModuleParams): .map(([k, v]) => set(values, k, v)) const valuesPath = getValuesPath(chartPath) - dumpYaml(valuesPath, values) + await dumpYaml(valuesPath, values) // keep track of which version has been built const buildVersionFilePath = join(buildPath, GARDEN_BUILD_VERSION_FILENAME) diff --git a/garden-cli/src/process.ts b/garden-cli/src/process.ts index 35c68d3b0a..1bd52adf48 100644 --- a/garden-cli/src/process.ts +++ b/garden-cli/src/process.ts @@ -95,9 +95,7 @@ export async function processModules( if (changedModule) { garden.log.debug({ msg: `Files changed for module ${changedModule.name}` }) - await Bluebird.map(changeHandler!(changedModule), task => { - garden.addTask(task) - }) + await Bluebird.map(changeHandler!(changedModule), (task) => garden.addTask(task)) } await garden.processTasks() diff --git a/garden-cli/src/task-graph.ts b/garden-cli/src/task-graph.ts index 162c449752..a272033985 100644 --- a/garden-cli/src/task-graph.ts +++ b/garden-cli/src/task-graph.ts @@ -7,6 +7,7 @@ */ import * as Bluebird from "bluebird" +import * as PQueue from "p-queue" import chalk from "chalk" import { merge, padEnd, pick } from "lodash" import { Task, TaskDefinitionError } from "./tasks/base" @@ -33,21 +34,8 @@ export interface TaskResults { [baseKey: string]: TaskResult } -interface LogEntryMap { [key: string]: LogEntry } - export const DEFAULT_CONCURRENCY = 4 -const taskStyle = chalk.cyan.bold - -function inProgressToStr(nodes) { - return `Currently in progress [${nodes.map(n => taskStyle(n.getKey())).join(", ")}]` -} - -function remainingTasksToStr(num) { - const style = num === 0 ? chalk.green : chalk.yellow - return `Remaining tasks ${style.bold(String(num))}` -} - export class TaskGraph { private roots: TaskNodeMap private index: TaskNodeMap @@ -56,22 +44,26 @@ export class TaskGraph { private logEntryMap: LogEntryMap private resultCache: ResultCache - private opQueue: OperationQueue + private opQueue: PQueue constructor(private garden: Garden, private concurrency: number = DEFAULT_CONCURRENCY) { this.roots = new TaskNodeMap() this.index = new TaskNodeMap() this.inProgress = new TaskNodeMap() this.resultCache = new ResultCache() - this.opQueue = new OperationQueue(this) + this.opQueue = new PQueue({ concurrency: 1 }) this.logEntryMap = {} } - addTask(task: Task): Promise { - return this.opQueue.request({ type: "addTask", task }) + async addTask(task: Task): Promise { + return this.opQueue.add(() => this.addTaskInternal(task)) } - async addTaskInternal(task: Task) { + async processTasks(): Promise { + return this.opQueue.add(() => this.processTasksInternal()) + } + + private async addTaskInternal(task: Task) { const predecessor = this.getPredecessor(task) let node = this.getNode(task) @@ -109,16 +101,10 @@ export class TaskGraph { const existing = this.index.getNode(task) return existing || new TaskNode(task) } - - processTasks(): Promise { - return this.opQueue.request({ type: "processTasks" }) - } - /* Process the graph until it's complete */ - async processTasksInternal(): Promise { - + private async processTasksInternal(): Promise { const _this = this const results: TaskResults = {} @@ -480,80 +466,15 @@ class ResultCache { } -// TODO: Add more typing to this class. - -/* - Used by TaskGraph to prevent race conditions e.g. when calling addTask or - processTasks. -*/ -class OperationQueue { - queue: object[] - draining: boolean - - constructor(private taskGraph: TaskGraph) { - this.queue = [] - this.draining = false - } - - request(opRequest): Promise { - let findFn - - switch (opRequest.type) { - - case "addTask": - findFn = (o) => o.type === "addTask" && o.task.getBaseKey() === opRequest.task.getBaseKey() - break - - case "processTasks": - findFn = (o) => o.type === "processTasks" - break - } - - const existingOp = this.queue.find(findFn) - - const prom = new Promise((resolver) => { - if (existingOp) { - existingOp["resolvers"].push(resolver) - } else { - this.queue.push({ ...opRequest, resolvers: [resolver] }) - } - }) - - if (!this.draining) { - this.process() - } - - return prom - } - - async process() { - this.draining = true - const op = this.queue.shift() - - if (!op) { - this.draining = false - return - } - - switch (op["type"]) { - - case "addTask": - const task = op["task"] - await this.taskGraph.addTaskInternal(task) - for (const resolver of op["resolvers"]) { - resolver() - } - break +interface LogEntryMap { [key: string]: LogEntry } - case "processTasks": - const results = await this.taskGraph.processTasksInternal() - for (const resolver of op["resolvers"]) { - resolver(results) - } - break - } +const taskStyle = chalk.cyan.bold - this.process() - } +function inProgressToStr(nodes) { + return `Currently in progress [${nodes.map(n => taskStyle(n.getKey())).join(", ")}]` +} +function remainingTasksToStr(num) { + const style = num === 0 ? chalk.green : chalk.yellow + return `Remaining tasks ${style.bold(String(num))}` } diff --git a/garden-cli/src/vcs/git.ts b/garden-cli/src/vcs/git.ts index 480e9b8bd9..899bfad631 100644 --- a/garden-cli/src/vcs/git.ts +++ b/garden-cli/src/vcs/git.ts @@ -9,7 +9,6 @@ import execa = require("execa") import { join } from "path" import { ensureDir, pathExists, stat } from "fs-extra" -import { argv } from "process" import Bluebird = require("bluebird") import { NEW_MODULE_VERSION, VcsHandler, RemoteSourceParams } from "./base" @@ -140,14 +139,3 @@ export class GitHandler extends VcsHandler { } } - -// used by the build process to resolve and store the tree version for plugin modules -if (require.main === module) { - const path = argv[2] - const handler = new GitHandler(path) - - handler.getTreeVersion(path) - .then((treeVersion) => { - console.log(JSON.stringify(treeVersion, null, 4)) - }) -} diff --git a/garden-cli/src/watch.ts b/garden-cli/src/watch.ts index 39e70ccb3f..d8aea84139 100644 --- a/garden-cli/src/watch.ts +++ b/garden-cli/src/watch.ts @@ -149,7 +149,7 @@ export class FSWatcher { if (configChanged) { // The added/removed dir contains one or more garden.yml files - this.invalidateCachedForAll() + await this.invalidateCachedForAll() return changeHandler(null, true) } diff --git a/tslint.json b/tslint.json index a37345addc..6c55b68047 100644 --- a/tslint.json +++ b/tslint.json @@ -36,6 +36,7 @@ "no-console": false, "no-debugger": true, "no-eval": true, + "no-floating-promises": true, "no-invalid-template-strings": true, "no-invalid-this": true, "no-null-keyword": false,