-
Notifications
You must be signed in to change notification settings - Fork 115
Maven combined detector experiment #1628
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
base: main
Are you sure you want to change the base?
Changes from all commits
7571467
ade981d
d3ba534
08a49df
61e7a58
2f2ba43
7faa9cc
5453e03
7302f96
db40e0e
d852f8c
b792b3e
bc79ea8
28dd56d
32f861e
009a0fc
6d25f86
e9a75da
3884939
1103f0c
6379015
3ff94b0
9056bd2
5e48830
5f4b641
27ccd95
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| namespace Microsoft.ComponentDetection.Detectors.Maven; | ||
|
|
||
| /// <summary> | ||
| /// Represents the result of executing a Maven CLI command. | ||
| /// </summary> | ||
| /// <param name="Success">Whether the command completed successfully.</param> | ||
| /// <param name="ErrorOutput">The error output from the command, if any.</param> | ||
| public record MavenCliResult(bool Success, string? ErrorOutput); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| namespace Microsoft.ComponentDetection.Detectors.Maven; | ||
|
|
||
| using System; | ||
| using System.Collections.Concurrent; | ||
| using System.IO; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
@@ -19,6 +20,19 @@ public class MavenCommandService : IMavenCommandService | |
|
|
||
| internal static readonly string[] AdditionalValidCommands = ["mvn.cmd"]; | ||
|
|
||
| /// <summary> | ||
| /// Per-location semaphores to prevent concurrent Maven CLI executions for the same pom.xml. | ||
| /// This allows multiple detectors (e.g., MvnCliComponentDetector and MavenWithFallbackDetector) | ||
| /// to safely share the same output file without race conditions. | ||
| /// </summary> | ||
| private readonly ConcurrentDictionary<string, SemaphoreSlim> locationLocks = new(); | ||
|
|
||
| /// <summary> | ||
| /// Tracks locations where dependency generation has completed successfully. | ||
| /// Used to skip duplicate executions when multiple detectors process the same pom.xml. | ||
| /// </summary> | ||
| private readonly ConcurrentDictionary<string, MavenCliResult> completedLocations = new(); | ||
|
|
||
zhenghao104 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| private readonly ICommandLineInvocationService commandLineInvocationService; | ||
| private readonly IMavenStyleDependencyGraphParserService parserService; | ||
| private readonly IEnvironmentVariableService envVarService; | ||
|
|
@@ -43,7 +57,57 @@ public async Task<bool> MavenCLIExistsAsync() | |
| return await this.commandLineInvocationService.CanCommandBeLocatedAsync(PrimaryCommand, AdditionalValidCommands, MvnVersionArgument); | ||
| } | ||
|
|
||
| public async Task GenerateDependenciesFileAsync(ProcessRequest processRequest, CancellationToken cancellationToken = default) | ||
| public async Task<MavenCliResult> GenerateDependenciesFileAsync(ProcessRequest processRequest, CancellationToken cancellationToken = default) | ||
| { | ||
| var pomFile = processRequest.ComponentStream; | ||
| var pomDir = Path.GetDirectoryName(pomFile.Location); | ||
| var depsFilePath = Path.Combine(pomDir, this.BcdeMvnDependencyFileName); | ||
|
|
||
| // Check the cache before acquiring the semaphore to allow fast-path returns | ||
| // even when cancellation has been requested. | ||
| if (this.completedLocations.TryGetValue(pomFile.Location, out var cachedResult) | ||
| && cachedResult.Success | ||
| && File.Exists(depsFilePath)) | ||
| { | ||
| this.logger.LogDebug("{DetectorPrefix}: Skipping duplicate \"dependency:tree\" for {PomFileLocation}, already generated", DetectorLogPrefix, pomFile.Location); | ||
| return cachedResult; | ||
| } | ||
|
|
||
| // Use semaphore to prevent concurrent Maven CLI executions for the same pom.xml. | ||
| // This allows MvnCliComponentDetector and MavenWithFallbackDetector to safely share the output file. | ||
| var semaphore = this.locationLocks.GetOrAdd(pomFile.Location, _ => new SemaphoreSlim(1, 1)); | ||
|
|
||
| await semaphore.WaitAsync(cancellationToken); | ||
| try | ||
zhenghao104 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| // Re-check the cache after acquiring the semaphore in case another caller | ||
| // completed while we were waiting. | ||
| if (this.completedLocations.TryGetValue(pomFile.Location, out cachedResult) | ||
| && cachedResult.Success | ||
| && File.Exists(depsFilePath)) | ||
| { | ||
|
Comment on lines
23
to
88
|
||
| this.logger.LogDebug("{DetectorPrefix}: Skipping duplicate \"dependency:tree\" for {PomFileLocation}, already generated", DetectorLogPrefix, pomFile.Location); | ||
| return cachedResult; | ||
| } | ||
|
|
||
| var result = await this.GenerateDependenciesFileCoreAsync(processRequest, cancellationToken); | ||
|
|
||
| // Only cache successful results. Failed results should allow retries for transient failures, | ||
| // and caching them would waste memory since the cache check requires Success == true anyway. | ||
| if (result.Success) | ||
| { | ||
| this.completedLocations[pomFile.Location] = result; | ||
| } | ||
|
|
||
| return result; | ||
|
Comment on lines
92
to
102
|
||
| } | ||
| finally | ||
| { | ||
| semaphore.Release(); | ||
| } | ||
| } | ||
|
|
||
| private async Task<MavenCliResult> GenerateDependenciesFileCoreAsync(ProcessRequest processRequest, CancellationToken cancellationToken) | ||
| { | ||
| var cliFileTimeout = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); | ||
| var timeoutSeconds = -1; | ||
|
|
@@ -58,7 +122,7 @@ public async Task GenerateDependenciesFileAsync(ProcessRequest processRequest, C | |
| var pomFile = processRequest.ComponentStream; | ||
| this.logger.LogDebug("{DetectorPrefix}: Running \"dependency:tree\" on {PomFileLocation}", DetectorLogPrefix, pomFile.Location); | ||
|
|
||
| var cliParameters = new[] { "dependency:tree", "-B", $"-DoutputFile={this.BcdeMvnDependencyFileName}", "-DoutputType=text", $"-f{pomFile.Location}" }; | ||
| string[] cliParameters = ["dependency:tree", "-B", $"-DoutputFile={this.BcdeMvnDependencyFileName}", "-DoutputType=text", $"-f{pomFile.Location}"]; | ||
|
|
||
| var result = await this.commandLineInvocationService.ExecuteCommandAsync(PrimaryCommand, AdditionalValidCommands, cancellationToken: cliFileTimeout.Token, cliParameters); | ||
|
|
||
|
|
@@ -78,10 +142,13 @@ public async Task GenerateDependenciesFileAsync(ProcessRequest processRequest, C | |
| { | ||
| this.logger.LogWarning("{DetectorPrefix}: There was a timeout in {PomFileLocation} file. Increase it using {TimeoutVar} environment variable.", DetectorLogPrefix, pomFile.Location, MvnCLIFileLevelTimeoutSecondsEnvVar); | ||
| } | ||
|
|
||
| return new MavenCliResult(false, errorMessage); | ||
| } | ||
| else | ||
| { | ||
| this.logger.LogDebug("{DetectorPrefix}: Execution of \"dependency:tree\" on {PomFileLocation} completed successfully", DetectorLogPrefix, pomFile.Location); | ||
| return new MavenCliResult(true, null); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The per-location
locationLocksandcompletedLocationsdictionaries grow monotonically and are never pruned, and theSemaphoreSliminstances are never disposed. SinceMavenCommandServiceis registered as a singleton, repeated scans (or very large repos) can cause unbounded memory growth.Consider using a bounded/expiring cache and removing/disposing per-location semaphores once generation is complete (or switching to a keyed lock implementation that doesn't retain entries indefinitely).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a clean up method in
OnDetectionFinishedAsyncto clear cache