Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions docs/reqstream/api-mark-dot-net.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ sections:
- ApiMarkDotNet-DotNetEmitter-EmitConversionOperatorsWithConversionSyntax
- ApiMarkDotNet-DotNetEmitter-RenderClassModifiersInTypeSignature
- ApiMarkDotNet-DotNetEmitter-RenderInitOnlyPropertyAccessor
- ApiMarkDotNet-DotNetEmitter-RenderConstantFieldValue
- ApiMarkDotNet-DotNetEmitterGradualDisclosure-EmitApiIndexPage
- ApiMarkDotNet-DotNetEmitterGradualDisclosure-EmitNamespacePages
- ApiMarkDotNet-DotNetEmitterGradualDisclosure-EmitNamespaceDocRemarksAndExamples
Expand Down
108 changes: 105 additions & 3 deletions src/ApiMark.Cpp/CppAst/ClangAstParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@
// clang the user has installed; callers can override via APIMARK_CLANG_PATH or
// an absolute ClangPath option. xcrun is only used here to probe for macOS
// Command Line Tools availability and is always a fixed system utility on PATH.
FileName = "xcrun",

Check warning on line 448 in src/ApiMark.Cpp/CppAst/ClangAstParser.cs

View workflow job for this annotation

GitHub Actions / Build / Build windows-latest

Use an absolute path for this command.
RedirectStandardOutput = true,
RedirectStandardError = true,
UseShellExecute = false,
Expand Down Expand Up @@ -702,11 +702,22 @@
/// Starts a process, reads stdout and stderr concurrently to avoid pipe deadlock,
/// and returns the combined output with the exit code.
/// </summary>
/// <remarks>
/// The wait is bounded by <see cref="ClangTimeoutMilliseconds"/> so that a clang (or
/// <c>xcrun</c>) invocation that never exits — e.g. a first-run Xcode license prompt
/// blocked on stdin, or a process starved by heavy CI-runner contention — fails this
/// single call instead of hanging the calling test (and, transitively, the whole CI
/// job) indefinitely. The process (and any children it spawned) is killed on timeout.
/// The resulting exception carries the OS, PID, elapsed time, full command line, and
/// any partial stdout/stderr the process had already produced, so a hang can be
/// diagnosed from CI output alone without needing to reproduce it locally.
/// </remarks>
/// <param name="fileName">Executable to launch.</param>
/// <param name="arguments">Arguments to pass to the executable.</param>
/// <returns>A tuple of (stdout, stderr, exitCode).</returns>
/// <exception cref="InvalidOperationException">
/// Thrown when the process cannot be started.
/// Thrown when the process cannot be started, or when it does not exit within
/// <see cref="ClangTimeoutMilliseconds"/>.
/// </exception>
private static (string Stdout, string Stderr, int ExitCode) RunProcess(
string fileName,
Expand All @@ -726,19 +737,110 @@
psi.ArgumentList.Add(arg);
}

var commandLine = BuildDiagnosticCommandLine(fileName, arguments);
var stopwatch = Stopwatch.StartNew();

using var process = Process.Start(psi)
?? throw new InvalidOperationException($"Failed to start process '{fileName}'.");
?? throw new InvalidOperationException(
$"Failed to start process '{fileName}'. Command: {commandLine}");

// Capture the PID up front — it is still readable after Kill(), but not reliably
// so once the process has fully exited and been reaped on some platforms
var processId = process.Id;

// Read stdout and stderr concurrently — reading them sequentially can deadlock
// when one pipe's buffer fills while the other is not being drained
var stdoutTask = process.StandardOutput.ReadToEndAsync();
var stderrTask = process.StandardError.ReadToEndAsync();

process.WaitForExit();
if (!process.WaitForExit(ClangTimeoutMilliseconds))
{
TryKillProcess(process);

// Best-effort: give the now-killed process's pipes a short grace period to flush
// so partial output (e.g. a diagnostic clang had already printed) can be reported.
// This wait is bounded independently of ClangTimeoutMilliseconds so a pipe that
// never closes cannot extend the hang.
var partialStdout = TryGetPartialOutput(stdoutTask);
var partialStderr = TryGetPartialOutput(stderrTask);
Comment thread
Malcolmnixon marked this conversation as resolved.
Resolved
Comment thread
Malcolmnixon marked this conversation as resolved.
Resolved

throw new InvalidOperationException(
$"Process did not exit within {ClangTimeoutMilliseconds}ms and was killed.\n" +
$" OS: {RuntimeInformation.OSDescription} ({RuntimeInformation.OSArchitecture})\n" +
$" PID: {processId}\n" +
$" Elapsed: {stopwatch.Elapsed}\n" +
$" Command: {commandLine}\n" +
$" Partial stdout ({partialStdout.Length} chars): {Truncate(partialStdout)}\n" +
$" Partial stderr ({partialStderr.Length} chars): {Truncate(partialStderr)}");
}

return (stdoutTask.Result, stderrTask.Result, process.ExitCode);
}

/// <summary>
/// Maximum time to wait for the clang (or <c>xcrun</c>-wrapped clang) AST-dump
/// invocation to exit before treating it as hung and killing it.
/// </summary>
private const int ClangTimeoutMilliseconds = 120_000;

/// <summary>
/// Maximum time to wait, after a hung process has been killed, for its redirected
/// stdout/stderr pipes to report as complete so partial output can be captured.
/// </summary>
private const int PartialOutputGraceMilliseconds = 2000;

/// <summary>
/// Maximum number of characters of partial stdout/stderr to include in a timeout
/// exception message, so a chatty hung process does not produce an unreadable error.
/// </summary>
private const int PartialOutputMaxChars = 2000;

/// <summary>
/// Builds a human-readable, shell-quoted rendering of a command line for diagnostic
/// messages. Not used to actually invoke the process — <see cref="ProcessStartInfo.ArgumentList"/>
/// already handles that safely.
/// </summary>
/// <param name="fileName">Executable name or path.</param>
/// <param name="arguments">Arguments passed to the executable.</param>
/// <returns>A single-line, space-separated, quoted command line for logging.</returns>
private static string BuildDiagnosticCommandLine(string fileName, IReadOnlyList<string> arguments)
{
var parts = new[] { fileName }.Concat(arguments)
.Select(a => a.Contains(' ', StringComparison.Ordinal) ? $"\"{a}\"" : a);
Comment thread
Malcolmnixon marked this conversation as resolved.
Resolved
return string.Join(' ', parts);
}

/// <summary>
/// Best-effort retrieval of a stdout/stderr read task's result after the owning
/// process has been killed, bounded by <see cref="PartialOutputGraceMilliseconds"/> so
/// a pipe that never reports complete cannot extend the timeout exception path.
/// </summary>
/// <param name="readTask">The <see cref="StreamReader.ReadToEndAsync()"/> task to probe.</param>
/// <returns>The captured text, or an empty string if it did not complete in time.</returns>
private static string TryGetPartialOutput(Task<string> readTask)
{
try
{
return readTask.Wait(PartialOutputGraceMilliseconds) ? readTask.Result : string.Empty;
}
catch (AggregateException)
{
// The read itself faulted (e.g. pipe torn down mid-read) — no partial output available
return string.Empty;
}
}

/// <summary>
/// Truncates diagnostic text to <see cref="PartialOutputMaxChars"/> so a chatty hung
/// process cannot produce an unreadable exception message.
/// </summary>
/// <param name="text">The text to truncate.</param>
/// <returns>The original text, or a truncated copy with a trailing marker.</returns>
private static string Truncate(string text) =>
text.Length <= PartialOutputMaxChars
? text
: string.Concat(text.AsSpan(0, PartialOutputMaxChars), "… [truncated]");

/// <summary>
/// Extracts error-class lines from clang's stderr output, used to populate
/// <see cref="CppCompilationResult.Errors"/>.
Expand Down Expand Up @@ -1151,7 +1253,7 @@
/// <param name="isDeprecated">Whether the class is deprecated, set in place by a <c>DeprecatedAttr</c> child.</param>
/// <param name="isFinal">Whether the class is final, set in place by a <c>FinalAttr</c> child.</param>
/// <param name="doc">The class documentation comment, set in place by a <c>FullComment</c> child.</param>
private void ProcessClassBodyChild(

Check warning on line 1256 in src/ApiMark.Cpp/CppAst/ClangAstParser.cs

View workflow job for this annotation

GitHub Actions / Build / Build windows-latest

Method has 13 parameters, which is greater than the 7 authorized.
JsonElement child,
string nsQualName,
string className,
Expand Down Expand Up @@ -1789,7 +1891,7 @@
/// <param name="node">The field JSON node.</param>
/// <param name="accessibility">The current access level at the point of declaration.</param>
/// <returns>A populated <see cref="CppField"/>, or <see langword="null"/> for implicit members.</returns>
private static CppField? ParseField(JsonElement node, CppAccessibility accessibility)

Check warning on line 1894 in src/ApiMark.Cpp/CppAst/ClangAstParser.cs

View workflow job for this annotation

GitHub Actions / Build / Build windows-latest

Refactor this method to reduce its Cognitive Complexity from 19 to the 15 allowed.
{
// Skip compiler-synthesized fields
if (node.TryGetProperty(IsImplicitProperty, out var impl) && impl.GetBoolean())
Expand Down Expand Up @@ -1885,7 +1987,7 @@
// Numeric and string literals carry their value as a JSON string, already formatted
// the way clang would print them (strings already include surrounding quotes)
"IntegerLiteral" or "FloatingLiteral" or "StringLiteral" =>
node.TryGetProperty("value", out var v) ? v.GetString() : null,

Check warning on line 1990 in src/ApiMark.Cpp/CppAst/ClangAstParser.cs

View workflow job for this annotation

GitHub Actions / Build / Build windows-latest

Define a constant instead of using this literal 'value' 5 times.

// Boolean literals carry their value as a JSON boolean, not a string
"CXXBoolLiteralExpr" => ExtractBoolLiteralDefaultValue(node),
Expand Down
Loading