From ffc53a9ebfd460804da4952fc93879e6d32ee582 Mon Sep 17 00:00:00 2001 From: Yassin Kammoun <52890329+yassin-kammoun-sonarsource@users.noreply.github.com> Date: Mon, 16 Oct 2023 12:12:48 +0200 Subject: [PATCH] Handle out-of-memory errors in the worker thread (#4273) --- .../src/test/java/SonarLintTest.java | 4 +- .../it/plugin/YamlAnalysisTest.java | 4 +- packages/bridge/src/errors/middleware.ts | 7 +- packages/bridge/src/memory.ts | 78 +++++++++++++++++++ packages/bridge/src/server.ts | 47 ++++------- packages/bridge/tests/memory.test.ts | 57 ++++++++++++++ packages/jsts/src/linter/issues/message.ts | 3 +- packages/jsts/src/program/program.ts | 13 +++- packages/shared/src/helpers/index.ts | 2 +- .../src/helpers/{debug.ts => logging.ts} | 4 + .../{debug.test.ts => logging.test.ts} | 11 ++- .../bridge/AbstractBridgeSensor.java | 2 +- .../bridge/AnalysisWithProgram.java | 3 - .../bridge/AnalysisWithWatchProgram.java | 10 +-- .../javascript/bridge/BridgeServerImpl.java | 22 +++--- .../plugins/javascript/bridge/BundleImpl.java | 4 +- .../javascript/bridge/CssRuleSensor.java | 4 - .../plugins/javascript/bridge/HtmlSensor.java | 12 +-- .../plugins/javascript/bridge/YamlSensor.java | 8 +- .../javascript/nodejs/NodeCommand.java | 4 +- .../bridge/BridgeServerImplTest.java | 24 +++--- .../javascript/bridge/CssRuleSensorTest.java | 9 --- .../javascript/bridge/HtmlSensorTest.java | 16 ---- .../JavaScriptEslintBasedSensorTest.java | 18 +---- .../javascript/bridge/JsTsSensorTest.java | 15 ---- .../javascript/bridge/YamlSensorTest.java | 16 ---- .../javascript/nodejs/NodeCommandTest.java | 5 +- 27 files changed, 224 insertions(+), 178 deletions(-) create mode 100644 packages/bridge/src/memory.ts create mode 100644 packages/bridge/tests/memory.test.ts rename packages/shared/src/helpers/{debug.ts => logging.ts} (95%) rename packages/shared/tests/helpers/{debug.test.ts => logging.test.ts} (84%) diff --git a/its/plugin/sonarlint-tests/src/test/java/SonarLintTest.java b/its/plugin/sonarlint-tests/src/test/java/SonarLintTest.java index ae93aaca555..94bb253283b 100644 --- a/its/plugin/sonarlint-tests/src/test/java/SonarLintTest.java +++ b/its/plugin/sonarlint-tests/src/test/java/SonarLintTest.java @@ -122,9 +122,9 @@ private static boolean usingEmbeddedNode() { @Test void should_start_node_server_once() throws Exception { analyze(FILE_PATH, ""); - assertThat(logs).doesNotContain("the bridge server is up, no need to start."); + assertThat(logs).doesNotContain("The bridge server is up, no need to start."); analyze(FILE_PATH, ""); - assertThat(logs).contains("the bridge server is up, no need to start."); + assertThat(logs).contains("The bridge server is up, no need to start."); } @Test diff --git a/its/plugin/tests/src/test/java/com/sonar/javascript/it/plugin/YamlAnalysisTest.java b/its/plugin/tests/src/test/java/com/sonar/javascript/it/plugin/YamlAnalysisTest.java index 249da1ad67b..ee546b9483a 100644 --- a/its/plugin/tests/src/test/java/com/sonar/javascript/it/plugin/YamlAnalysisTest.java +++ b/its/plugin/tests/src/test/java/com/sonar/javascript/it/plugin/YamlAnalysisTest.java @@ -64,7 +64,7 @@ void single_line_inline_aws_lambda_for_js() throws IOException { assertThat(issuesList) .extracting(Issue::getLine, Issue::getRule) .containsExactlyInAnyOrder(tuple(12, "javascript:S3923")); - assertThat(result.getLogsLines(log -> log.contains("Starting Node.js process"))).hasSize(1); + assertThat(result.getLogsLines(log -> log.contains("Creating Node.js process"))).hasSize(1); // assertPerfMonitoringAvailable(perfMonitoringDir); } @@ -89,6 +89,6 @@ void dont_start_eslint_bridge_for_yaml_without_nodejs_aws() { OrchestratorStarter.setProfiles(projectKey, Map.of("eslint-based-rules-profile", "js")); BuildResult result = orchestrator.executeBuild(build); - assertThat(result.getLogsLines(log -> log.contains("Starting Node.js process"))).hasSize(0); + assertThat(result.getLogsLines(log -> log.contains("Creating Node.js process"))).hasSize(0); } } diff --git a/packages/bridge/src/errors/middleware.ts b/packages/bridge/src/errors/middleware.ts index 2a0512c4e92..ea915777a0c 100644 --- a/packages/bridge/src/errors/middleware.ts +++ b/packages/bridge/src/errors/middleware.ts @@ -20,6 +20,7 @@ import express from 'express'; import { ErrorCode } from '@sonar/shared/errors'; import { JsTsAnalysisOutput } from '@sonar/jsts'; +import { error } from '@sonar/shared/helpers'; /** * Express.js middleware for handling error while serving requests. @@ -32,12 +33,12 @@ import { JsTsAnalysisOutput } from '@sonar/jsts'; * @see https://expressjs.com/en/guide/error-handling.html */ export function errorMiddleware( - error: any, + err: any, _request: express.Request, response: express.Response, _next: express.NextFunction, ) { - const { code, message, stack, data } = error; + const { code, message, stack, data } = err; switch (code) { case ErrorCode.Parsing: response.json({ @@ -59,7 +60,7 @@ export function errorMiddleware( }); break; default: - console.error(stack); + error(stack); response.json({ error: message }); break; } diff --git a/packages/bridge/src/memory.ts b/packages/bridge/src/memory.ts new file mode 100644 index 00000000000..b29d6afb409 --- /dev/null +++ b/packages/bridge/src/memory.ts @@ -0,0 +1,78 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) 2011-2023 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +import * as v8 from 'v8'; +import * as os from 'os'; +import fs from 'fs'; +import { error, info, warn } from '@sonar/shared/helpers'; + +const MB = 1024 * 1024; + +export function logMemoryConfiguration() { + const osMem = Math.floor(os.totalmem() / MB); + const heapSize = getHeapSize(); + const dockerMemLimit = readDockerMemoryLimit(); + const dockerMem = dockerMemLimit ? `Docker mem: ${dockerMemLimit} MB. ` : ''; + info(`OS memory ${osMem} MB. ${dockerMem}Node.js heap size limit: ${heapSize} MB.`); + if (heapSize > osMem) { + warn( + `Node.js heap size limit ${heapSize} is higher than available memory ${osMem}. Check your configuration of sonar.javascript.node.maxspace`, + ); + } +} + +function readDockerMemoryLimit() { + try { + const mem = Number.parseInt(fs.readFileSync('/sys/fs/cgroup/memory.max', { encoding: 'utf8' })); + if (Number.isInteger(mem)) { + return mem; + } + } catch (e) { + // probably not a docker env + } + return undefined; +} + +function getHeapSize() { + return Math.floor(v8.getHeapStatistics().heap_size_limit / MB); +} + +export function logMemoryError(err: any) { + switch (err?.code) { + case 'ERR_WORKER_OUT_OF_MEMORY': + error( + `The analysis will stop due to the Node.js process running out of memory (heap size limit ${getHeapSize()} MB)`, + ); + error( + `You can see how Node.js heap usage evolves during analysis with "sonar.javascript.node.debugMemory=true"`, + ); + error( + 'Try setting "sonar.javascript.node.maxspace" to a higher value to increase Node.js heap size limit', + ); + error( + 'If the problem persists, please report the issue at https://community.sonarsource.com', + ); + break; + default: + error(`The analysis will stop due to an unexpected error: ${err}`); + error('Please report the issue at https://community.sonarsource.com'); + break; + } +} diff --git a/packages/bridge/src/server.ts b/packages/bridge/src/server.ts index 9b6f5aea682..c0893d26d6f 100644 --- a/packages/bridge/src/server.ts +++ b/packages/bridge/src/server.ts @@ -17,6 +17,7 @@ * along with this program; if not, write to the Free Software Foundation, * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ + /** * `module-alias` must be imported first for module aliasing to work. */ @@ -27,13 +28,11 @@ import http from 'http'; import path from 'path'; import router from './router'; import { errorMiddleware } from './errors'; -import { debug, getContext, info, warn } from '@sonar/shared/helpers'; +import { debug, getContext } from '@sonar/shared/helpers'; import { timeoutMiddleware } from './timeout'; import { AddressInfo } from 'net'; -import * as v8 from 'v8'; -import * as os from 'os'; import { Worker } from 'worker_threads'; -import fs from 'fs'; +import { logMemoryConfiguration, logMemoryError } from './memory'; /** * The maximum request body size @@ -49,33 +48,6 @@ const MAX_REQUEST_SIZE = '50mb'; */ const SHUTDOWN_TIMEOUT = 15_000; -const MB = 1024 * 1024; - -function logMemoryConfiguration() { - const osMem = Math.floor(os.totalmem() / MB); - const heapSize = Math.floor(v8.getHeapStatistics().heap_size_limit / MB); - const dockerMemLimit = readDockerMemoryLimit(); - const dockerMem = dockerMemLimit ? `Docker mem: ${dockerMemLimit} MB. ` : ''; - info(`OS memory ${osMem} MB. ${dockerMem}Node.js heap size limit: ${heapSize} MB.`); - if (heapSize > osMem) { - warn( - `Node.js heap size limit ${heapSize} is higher than available memory ${osMem}. Check your configuration of sonar.javascript.node.maxspace`, - ); - } -} - -function readDockerMemoryLimit() { - try { - const mem = Number.parseInt(fs.readFileSync('/sys/fs/cgroup/memory.max', { encoding: 'utf8' })); - if (Number.isInteger(mem)) { - return mem; - } - } catch (e) { - // probably not a docker env - } - return undefined; -} - /** * A pool of a single worker thread * @@ -128,6 +100,19 @@ export function start( worker.on('error', err => { debug(`The worker thread failed: ${err}`); + + logMemoryError(err); + + /** + * At this point, the worker thread can no longer respond to any request from the plugin. + * However, existing requests are stalled until they time out. Since the bridge server is + * about to be shut down in an unexpected manner anyway, we can close all connections and + * avoid waiting unnecessarily for them to eventually close. + */ + server.closeAllConnections(); + + debug('Shutting down the bridge server due to failure'); + shutdown(); }); const app = express(); diff --git a/packages/bridge/tests/memory.test.ts b/packages/bridge/tests/memory.test.ts new file mode 100644 index 00000000000..08e2c64dbce --- /dev/null +++ b/packages/bridge/tests/memory.test.ts @@ -0,0 +1,57 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) 2011-2023 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +import { logMemoryError } from '../src/memory'; + +describe('logMemoryError', () => { + it('should log out-of-memory troubleshooting guide', () => { + console.error = jest.fn(); + + logMemoryError({ code: 'ERR_WORKER_OUT_OF_MEMORY' }); + + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining( + `The analysis will stop due to the Node\.js process running out of memory`, + ), + ); + expect(console.error).toHaveBeenCalledWith( + 'You can see how Node.js heap usage evolves during analysis with "sonar.javascript.node.debugMemory=true"', + ); + expect(console.error).toHaveBeenCalledWith( + 'Try setting "sonar.javascript.node.maxspace" to a higher value to increase Node.js heap size limit', + ); + expect(console.error).toHaveBeenCalledWith( + 'If the problem persists, please report the issue at https://community.sonarsource.com', + ); + }); + + it('should log default troubleshooting guide', () => { + console.error = jest.fn(); + + logMemoryError('something failed'); + + expect(console.error).toHaveBeenCalledWith( + 'The analysis will stop due to an unexpected error: something failed', + ); + expect(console.error).toHaveBeenCalledWith( + 'Please report the issue at https://community.sonarsource.com', + ); + }); +}); diff --git a/packages/jsts/src/linter/issues/message.ts b/packages/jsts/src/linter/issues/message.ts index f86551595da..bc069fb59f3 100644 --- a/packages/jsts/src/linter/issues/message.ts +++ b/packages/jsts/src/linter/issues/message.ts @@ -20,6 +20,7 @@ import { Linter, SourceCode } from 'eslint'; import { transformFixes } from '../quickfixes'; import { Issue } from './issue'; +import { error } from '@sonar/shared/helpers'; /** * Converts an ESLint message into a SonarQube issue @@ -40,7 +41,7 @@ export function convertMessage(source: SourceCode, message: Linter.LintMessage): * happen because we lint ready SourceCode instances and not file contents. */ if (!message.ruleId) { - console.error("Illegal 'null' ruleId for eslint issue"); + error("Illegal 'null' ruleId for eslint issue"); return null; } return { diff --git a/packages/jsts/src/program/program.ts b/packages/jsts/src/program/program.ts index a6be17edf69..3571f7731e3 100644 --- a/packages/jsts/src/program/program.ts +++ b/packages/jsts/src/program/program.ts @@ -30,7 +30,14 @@ import path from 'path'; import ts from 'typescript'; -import { addTsConfigIfDirectory, debug, readFileSync, toUnixPath } from '@sonar/shared/helpers'; +import { + addTsConfigIfDirectory, + debug, + error, + readFileSync, + toUnixPath, + warn, +} from '@sonar/shared/helpers'; import tmp from 'tmp'; import { promisify } from 'util'; import fs from 'fs/promises'; @@ -90,7 +97,7 @@ export function createProgramOptions( const config = ts.readConfigFile(tsConfig, parseConfigHost.readFile); if (config.error) { - console.error(`Failed to parse tsconfig: ${tsConfig} (${diagnosticToString(config.error)})`); + error(`Failed to parse tsconfig: ${tsConfig} (${diagnosticToString(config.error)})`); throw Error(diagnosticToString(config.error)); } @@ -153,7 +160,7 @@ export function createProgram(tsConfig: string, tsconfigContents?: string): Prog for (const reference of inputProjectReferences) { const sanitizedReference = addTsConfigIfDirectory(reference.path); if (!sanitizedReference) { - console.log(`WARN Skipping missing referenced tsconfig.json: ${reference.path}`); + warn(`Skipping missing referenced tsconfig.json: ${reference.path}`); } else { projectReferences.push(sanitizedReference); } diff --git a/packages/shared/src/helpers/index.ts b/packages/shared/src/helpers/index.ts index b8f4afffcce..e5c72b34040 100644 --- a/packages/shared/src/helpers/index.ts +++ b/packages/shared/src/helpers/index.ts @@ -18,7 +18,7 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ export * from './context'; -export * from './debug'; +export * from './logging'; export * from './files'; export * from './language'; export * from './escape'; diff --git a/packages/shared/src/helpers/debug.ts b/packages/shared/src/helpers/logging.ts similarity index 95% rename from packages/shared/src/helpers/debug.ts rename to packages/shared/src/helpers/logging.ts index f4837c698b3..0ae7854462f 100644 --- a/packages/shared/src/helpers/debug.ts +++ b/packages/shared/src/helpers/logging.ts @@ -29,6 +29,10 @@ export function debug(message: string) { console.log(`DEBUG ${message}`); } +export function error(message: string) { + console.error(message); +} + export function info(message: string) { console.log(message); } diff --git a/packages/shared/tests/helpers/debug.test.ts b/packages/shared/tests/helpers/logging.test.ts similarity index 84% rename from packages/shared/tests/helpers/debug.test.ts rename to packages/shared/tests/helpers/logging.test.ts index 5064d53f1e4..dbe3d64e74d 100644 --- a/packages/shared/tests/helpers/debug.test.ts +++ b/packages/shared/tests/helpers/logging.test.ts @@ -17,7 +17,8 @@ * along with this program; if not, write to the Free Software Foundation, * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -import { debug, info, warn } from '../../src/helpers'; + +import { debug, error, info, warn } from '../../src/helpers'; describe('debug', () => { it('should log with a `DEBUG` prefix', () => { @@ -27,6 +28,14 @@ describe('debug', () => { }); }); +describe('error', () => { + it('should log to stderr', () => { + console.error = jest.fn(); + error('hello, world!'); + expect(console.error).toHaveBeenCalledWith(`hello, world!`); + }); +}); + describe('warn', () => { it('should log with a `WARN` prefix', () => { console.log = jest.fn(); diff --git a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AbstractBridgeSensor.java b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AbstractBridgeSensor.java index 2610baade56..113b3e73031 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AbstractBridgeSensor.java +++ b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AbstractBridgeSensor.java @@ -92,7 +92,7 @@ public void execute(SensorContext context) { ); } } catch (Exception e) { - LOG.error("Failure during analysis, " + bridgeServer.getCommandInfo(), e); + LOG.error("Failure during analysis", e); if (contextUtils.failFast()) { throw new IllegalStateException( "Analysis failed (\"sonar.internal.analysis.failFast\"=true)", diff --git a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AnalysisWithProgram.java b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AnalysisWithProgram.java index 847fd56cb58..6176a6caa27 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AnalysisWithProgram.java +++ b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AnalysisWithProgram.java @@ -149,9 +149,6 @@ private void analyze(InputFile file, @Nullable TsProgram tsProgram) throws IOExc "Analysis interrupted because the SensorContext is in cancelled state" ); } - if (!bridgeServer.isAlive()) { - throw new IllegalStateException("the bridge server is not answering"); - } var cacheStrategy = CacheStrategies.getStrategyFor(context, file); if (cacheStrategy.isAnalysisRequired()) { try { diff --git a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AnalysisWithWatchProgram.java b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AnalysisWithWatchProgram.java index 509f559c7e4..bf0432655e6 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AnalysisWithWatchProgram.java +++ b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/AnalysisWithWatchProgram.java @@ -117,13 +117,9 @@ private void analyzeTsConfig(@Nullable TsConfigFile tsConfigFile, List deployedBundles) throws IOExc .collect(Collectors.joining(File.pathSeparator)); initNodeCommand(context, scriptFile, context.fileSystem().workDir(), bundles); - LOG.debug("Starting Node.js process to start the bridge server at port " + port); + LOG.debug("Creating Node.js process to start the bridge server on port " + port); nodeCommand.start(); if (!waitServerToStart(timeoutSeconds * 1000)) { status = Status.FAILED; - throw new NodeCommandException("Failed to start server (" + timeoutSeconds + "s timeout)"); + throw new NodeCommandException( + "Failed to start the bridge server (" + timeoutSeconds + "s timeout)" + ); } else { serverHasStarted(); } @@ -284,12 +286,12 @@ public void startServerLazily(SensorContext context) throws IOException { if (providedPort != 0) { port = providedPort; serverHasStarted(); - LOG.info("Will use existing Node.js process in port " + port); + LOG.info("Using existing Node.js process on port " + port); } try { if (isAlive()) { - LOG.debug("the bridge server is up, no need to start."); + LOG.debug("The bridge server is up, no need to start."); return; } else if (status == Status.STARTED) { status = Status.FAILED; @@ -390,11 +392,7 @@ private String request(String json, String endpoint) throws IOException { } catch (InterruptedException e) { throw handleInterruptedException(e, "Request " + endpoint + " was interrupted."); } catch (IOException e) { - String msg = - "the bridge Node.js process is unresponsive. This is most likely caused by process running out of memory." + - " Consider setting sonar.javascript.node.maxspace to higher value (e.g. 4096)."; - LOG.error(msg); - throw new IllegalStateException("the bridge is unresponsive", e); + throw new IllegalStateException("The bridge server is unresponsive", e); } } @@ -510,7 +508,7 @@ public void clean() { try { request("", "close"); } catch (IOException e) { - LOG.warn("Failed to close server", e); + LOG.warn("Failed to close the bridge server", e); } nodeCommand.waitFor(); nodeCommand = null; @@ -531,7 +529,7 @@ public String getCommandInfo() { if (nodeCommand == null) { return "Node.js command to start the bridge server was not built yet."; } else { - return "Node.js command to start the bridge was: " + nodeCommand; + return "Node.js command to start the bridge server was: " + nodeCommand; } } diff --git a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/BundleImpl.java b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/BundleImpl.java index d7a21a88d3c..611625c3dac 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/BundleImpl.java +++ b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/BundleImpl.java @@ -54,10 +54,10 @@ public BundleImpl() { @Override public void deploy(Path deployLocation) throws IOException { PROFILER.startDebug("Deploying bundle"); - LOG.debug("Deploying the bridge into {}", deployLocation); + LOG.debug("Deploying the bridge server into {}", deployLocation); InputStream bundle = getClass().getResourceAsStream(bundleLocation); if (bundle == null) { - throw new IllegalStateException("the bridge not found in plugin jar"); + throw new IllegalStateException("The bridge server was not found in the plugin jar"); } BundleUtils.extractFromClasspath(bundle, deployLocation); this.deployLocation = deployLocation; diff --git a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/CssRuleSensor.java b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/CssRuleSensor.java index 9ace7dced70..cb0928ed7d7 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/CssRuleSensor.java +++ b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/CssRuleSensor.java @@ -113,10 +113,6 @@ protected void analyzeFiles(List inputFiles) throws IOException { "Analysis interrupted because the SensorContext is in cancelled state" ); } - if (!bridgeServer.isAlive()) { - throw new IllegalStateException("the bridge server is not answering"); - } - analyzeFile(inputFile, context, rules); progressReport.nextFile(inputFile.absolutePath()); } diff --git a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/HtmlSensor.java b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/HtmlSensor.java index f605d6a8cb6..f88ad1325e7 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/HtmlSensor.java +++ b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/HtmlSensor.java @@ -85,14 +85,10 @@ protected void analyzeFiles(List inputFiles) throws IOException { "Analysis interrupted because the SensorContext is in cancelled state" ); } - if (bridgeServer.isAlive()) { - progressReport.nextFile(inputFile.absolutePath()); - var cacheStrategy = CacheStrategies.getStrategyFor(context, inputFile); - if (cacheStrategy.isAnalysisRequired()) { - analyze(inputFile, cacheStrategy); - } - } else { - throw new IllegalStateException("the bridge server is not answering"); + progressReport.nextFile(inputFile.absolutePath()); + var cacheStrategy = CacheStrategies.getStrategyFor(context, inputFile); + if (cacheStrategy.isAnalysisRequired()) { + analyze(inputFile, cacheStrategy); } } success = true; diff --git a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/YamlSensor.java b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/YamlSensor.java index ed969da6ab0..65cf12fa5a2 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/YamlSensor.java +++ b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/YamlSensor.java @@ -83,12 +83,8 @@ protected void analyzeFiles(List inputFiles) throws IOException { "Analysis interrupted because the SensorContext is in cancelled state" ); } - if (bridgeServer.isAlive()) { - progressReport.nextFile(inputFile.absolutePath()); - analyze(inputFile); - } else { - throw new IllegalStateException("the bridge server is not answering"); - } + progressReport.nextFile(inputFile.absolutePath()); + analyze(inputFile); } success = true; } finally { diff --git a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/nodejs/NodeCommand.java b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/nodejs/NodeCommand.java index de55ad171e0..9a1155de568 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/nodejs/NodeCommand.java +++ b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/nodejs/NodeCommand.java @@ -115,14 +115,14 @@ public int waitFor() { if (success) { exitValue = processWrapper.exitValue(process); } else { - LOG.error("Node process did not stop in a timely fashion"); + LOG.error("Node.js process did not stop in a timely fashion"); processWrapper.destroyForcibly(process); exitValue = -1; } return exitValue; } catch (InterruptedException e) { processWrapper.interrupt(); - LOG.error("Interrupted while waiting for process to terminate."); + LOG.error("Interrupted while waiting for Node.js process to terminate."); return 1; } } diff --git a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java index 0c81e1377da..f2281b43660 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java +++ b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java @@ -359,7 +359,7 @@ void should_throw_if_failed_to_start() throws Exception { assertThatThrownBy(() -> bridgeServer.startServer(context, deployedBundles)) .isInstanceOf(NodeCommandException.class) - .hasMessage("Failed to start server (" + TEST_TIMEOUT_SECONDS + "s timeout)"); + .hasMessage("Failed to start the bridge server (" + TEST_TIMEOUT_SECONDS + "s timeout)"); } @Test @@ -372,7 +372,7 @@ void should_return_command_info() throws Exception { bridgeServer.startServer(context, emptyList()); assertThat(bridgeServer.getCommandInfo()) - .contains("Node.js command to start the bridge was: ", "node", START_SERVER_SCRIPT); + .contains("Node.js command to start the bridge server was: ", "node", START_SERVER_SCRIPT); assertThat(bridgeServer.getCommandInfo()).doesNotContain("--max-old-space-size"); } @@ -424,8 +424,8 @@ void test_isAlive() throws Exception { @Test void test_lazy_start() throws Exception { - String alreadyStarted = "the bridge server is up, no need to start."; - String starting = "Starting Node.js process to start the bridge server at port"; + String alreadyStarted = "The bridge server is up, no need to start."; + String starting = "Creating Node.js process to start the bridge server on port"; bridgeServer = createBridgeServer("startServer.js"); bridgeServer.startServerLazily(context); assertThat(logTester.logs(DEBUG).stream().anyMatch(s -> s.startsWith(starting))).isTrue(); @@ -438,9 +438,9 @@ void test_lazy_start() throws Exception { @Test void test_use_existing_node() throws Exception { - String starting = "Starting Node.js process to start the bridge server at port"; - var useExisting = "Will use existing Node.js process in port 60000"; - var alreadyStarted = "the bridge server is up, no need to start."; + String starting = "Creating Node.js process to start the bridge server on port"; + var useExisting = "Using existing Node.js process on port 60000"; + var alreadyStarted = "The bridge server is up, no need to start."; var wrongPortRange = "Node.js process port set in $SONARJS_EXISTING_NODE_PROCESS_PORT should be a number between 1 and 65535 range"; var wrongPortValue = @@ -480,7 +480,7 @@ void test_use_existing_node() throws Exception { void should_throw_special_exception_when_failed_start_server_before() { bridgeServer = createBridgeServer("throw.js"); String failedToStartExceptionMessage = - "Failed to start server (" + TEST_TIMEOUT_SECONDS + "s timeout)"; + "Failed to start the bridge server (" + TEST_TIMEOUT_SECONDS + "s timeout)"; assertThatThrownBy(() -> bridgeServer.startServerLazily(context)) .isInstanceOf(NodeCommandException.class) .hasMessage(failedToStartExceptionMessage); @@ -613,13 +613,7 @@ void log_error_when_timeout() throws Exception { assertThatThrownBy(() -> bridgeServer.loadTsConfig("any.ts")) .isInstanceOf(IllegalStateException.class) - .hasMessage("the bridge is unresponsive"); - assertThat(logTester.logs(ERROR)) - .contains( - "the bridge Node.js process is unresponsive. This is most likely " + - "caused by process running out of memory. Consider setting sonar.javascript.node.maxspace to higher value" + - " (e.g. 4096)." - ); + .hasMessage("The bridge server is unresponsive"); } @Test diff --git a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/CssRuleSensorTest.java b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/CssRuleSensorTest.java index 36de52f95ad..d8a8df74074 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/CssRuleSensorTest.java +++ b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/CssRuleSensorTest.java @@ -301,15 +301,6 @@ void should_not_analyze_files_with_not_file_uri() throws URISyntaxException, IOE .doesNotMatch("(?s).*\nAnalyzing \\S*file.css.*"); } - @Test - void analysis_stop_when_server_is_not_anymore_alive() { - addInputFile("file.css"); - when(bridgeServerMock.isAlive()).thenReturn(false); - sensor.execute(context); - assertThat(logTester.logs(LoggerLevel.ERROR)) - .contains("Failure during analysis, bridgeServerMock command info"); - } - @Test void should_stop_execution_when_sensor_context_is_cancelled() { addInputFile("file.css"); diff --git a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/HtmlSensorTest.java b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/HtmlSensorTest.java index 442885cb4be..1ceb4d9a85d 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/HtmlSensorTest.java +++ b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/HtmlSensorTest.java @@ -252,22 +252,6 @@ void stop_analysis_if_cancelled() throws Exception { ); } - @Test - void stop_analysis_if_server_is_not_responding() throws Exception { - when(bridgeServerMock.isAlive()).thenReturn(false); - - HtmlSensor HtmlSensor = createSensor(); - createInputFile(context); - HtmlSensor.execute(context); - - final LogAndArguments logAndArguments = logTester.getLogs(Level.ERROR).get(0); - - assertThat(logAndArguments.getFormattedMsg()) - .isEqualTo("Failure during analysis, bridgeServerMock command info"); - assertThat(logAndArguments.getThrowable().getMessage()) - .isEqualTo("the bridge server is not answering"); - } - @Test void log_debug_analyzed_filename() throws Exception { when(bridgeServerMock.analyzeHtml(any())).thenReturn(new AnalysisResponse()); diff --git a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/JavaScriptEslintBasedSensorTest.java b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/JavaScriptEslintBasedSensorTest.java index 74ff841db3c..4c0a7068204 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/JavaScriptEslintBasedSensorTest.java +++ b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/JavaScriptEslintBasedSensorTest.java @@ -444,7 +444,7 @@ void should_save_cpd() throws Exception { @Test void should_catch_if_bridge_server_not_started() throws Exception { - doThrow(new IllegalStateException("failed to start server")) + doThrow(new IllegalStateException("Failed to start the bridge server")) .when(bridgeServerMock) .startServerLazily(context); @@ -452,8 +452,7 @@ void should_catch_if_bridge_server_not_started() throws Exception { createInputFile(context); sensor.execute(context); - assertThat(logTester.logs(LoggerLevel.ERROR)) - .contains("Failure during analysis, bridgeServerMock command info"); + assertThat(logTester.logs(LoggerLevel.ERROR)).contains("Failure during analysis"); assertThat(context.allIssues()).isEmpty(); } @@ -568,19 +567,6 @@ void log_debug_if_already_failed_server() throws Exception { ); } - @Test - void stop_analysis_if_server_is_not_responding() throws Exception { - when(bridgeServerMock.isAlive()).thenReturn(false); - var javaScriptEslintBasedSensor = createSensor(); - createInputFile(context); - javaScriptEslintBasedSensor.execute(context); - final var logAndArguments = logTester.getLogs(Level.ERROR).get(0); - assertThat(logAndArguments.getFormattedMsg()) - .isEqualTo("Failure during analysis, bridgeServerMock command info"); - assertThat(logAndArguments.getThrowable().getMessage()) - .isEqualTo("the bridge server is not answering"); - } - @Test void should_raise_a_parsing_error() throws IOException { when(bridgeServerMock.analyzeJavaScript(any())) diff --git a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/JsTsSensorTest.java b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/JsTsSensorTest.java index c5e9ad41fd8..3bfa7595f1c 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/JsTsSensorTest.java +++ b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/JsTsSensorTest.java @@ -694,21 +694,6 @@ void should_fail_fast_with_parsing_error_without_line() throws IOException { .contains("Failed to analyze file [dir/file.ts]: Parse error message"); } - @Test - void stop_analysis_if_server_is_not_responding() throws Exception { - when(bridgeServerMock.isAlive()).thenReturn(false); - JsTsSensor sensor = createSensor(); - createTsConfigFile(); - createVueInputFile(); - createInputFile(context); - sensor.execute(context); - final var logAndArguments = logTester.getLogs(Level.ERROR).get(0); - assertThat(logAndArguments.getFormattedMsg()) - .isEqualTo("Failure during analysis, bridgeServerMock command info"); - assertThat(logAndArguments.getThrowable().getMessage()) - .isEqualTo("the bridge server is not answering"); - } - @Test void stop_analysis_if_cancelled() throws Exception { JsTsSensor sensor = createSensor(); diff --git a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/YamlSensorTest.java b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/YamlSensorTest.java index 2325ce48662..a18fa39bff9 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/YamlSensorTest.java +++ b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/YamlSensorTest.java @@ -220,22 +220,6 @@ void stop_analysis_if_cancelled() throws Exception { ); } - @Test - void stop_analysis_if_server_is_not_responding() throws Exception { - when(bridgeServerMock.isAlive()).thenReturn(false); - - YamlSensor yamlSensor = createSensor(); - createInputFile(context); - yamlSensor.execute(context); - - final var logAndArguments = logTester.getLogs(Level.ERROR).get(0); - - assertThat(logAndArguments.getFormattedMsg()) - .isEqualTo("Failure during analysis, bridgeServerMock command info"); - assertThat(logAndArguments.getThrowable().getMessage()) - .isEqualTo("the bridge server is not answering"); - } - @Test void log_debug_analyzed_filename() throws Exception { when(bridgeServerMock.analyzeYaml(any())).thenReturn(new AnalysisResponse()); diff --git a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/nodejs/NodeCommandTest.java b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/nodejs/NodeCommandTest.java index 56826a81ddd..7f316c2569e 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/nodejs/NodeCommandTest.java +++ b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/nodejs/NodeCommandTest.java @@ -261,7 +261,8 @@ void test_interrupted_waitFor() throws Exception { nodeCommand.start(); int exitValue = nodeCommand.waitFor(); verify(mockProcessWrapper).interrupt(); - assertThat(logTester.logs()).contains("Interrupted while waiting for process to terminate."); + assertThat(logTester.logs()) + .contains("Interrupted while waiting for Node.js process to terminate."); assertThat(exitValue).isEqualTo(1); } @@ -274,7 +275,7 @@ void test_timeout_waitFor() throws Exception { nodeCommand.start(); int exitValue = nodeCommand.waitFor(); verify(mockProcessWrapper).destroyForcibly(any()); - assertThat(logTester.logs()).contains("Node process did not stop in a timely fashion"); + assertThat(logTester.logs()).contains("Node.js process did not stop in a timely fashion"); assertThat(exitValue).isEqualTo(-1); }