Skip to content

Commit d3f3005

Browse files
committed
Ensures Git streams are closed eagerly
- Prevents resource leaks and potential `SIGPIPE` errors by explicitly calling `return()` on stream generators - Adds detailed start and complete logging for Git stream commands
1 parent aad542d commit d3f3005

File tree

5 files changed

+59
-11
lines changed

5 files changed

+59
-11
lines changed

src/env/node/git/git.ts

Lines changed: 48 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,7 @@ function defaultExceptionHandler(ex: Error, cwd: string | undefined, start?: [nu
169169
}
170170

171171
const uniqueCounterForStdin = getScopedCounter();
172+
const uniqueCounterForStream = getScopedCounter();
172173

173174
type ExitCodeOnlyGitCommandOptions = GitCommandOptions & { exitCodeOnly: true };
174175
export type PushForceOptions = { withLease: true; ifIncludes?: boolean } | { withLease: false; ifIncludes?: never };
@@ -392,14 +393,15 @@ export class Git implements Disposable {
392393
exception = undefined;
393394
return { stdout: '' as T, stderr: result?.stderr as T | undefined, exitCode: result?.exitCode ?? 0 };
394395
} finally {
395-
this.logGitCommand(gitCommand, exception, getDurationMilliseconds(start), waiting);
396+
this.logGitCommandComplete(gitCommand, exception, getDurationMilliseconds(start), waiting);
396397
}
397398
}
398399

399400
async *stream(options: GitSpawnOptions, ...args: readonly (string | undefined)[]): AsyncGenerator<string> {
400401
if (!workspace.isTrusted) throw new WorkspaceUntrustedError();
401402

402403
const start = hrtime();
404+
const streamId = uniqueCounterForStream.next();
403405

404406
const { cancellation, configs, stdin, stdinEncoding, ...opts } = options;
405407
const runArgs = args.filter(a => a != null);
@@ -457,6 +459,7 @@ export class Git implements Disposable {
457459

458460
const command = await this.path();
459461
const proc = spawn(command, runArgs, spawnOpts);
462+
460463
if (stdin) {
461464
proc.stdin?.end(stdin, (stdinEncoding ?? 'utf8') as BufferEncoding);
462465
}
@@ -524,7 +527,23 @@ export class Git implements Disposable {
524527
});
525528
});
526529

530+
let cleanedUp = false;
531+
const cleanup = () => {
532+
if (cleanedUp) return;
533+
cleanedUp = true;
534+
535+
try {
536+
disposable?.dispose();
537+
} catch {}
538+
try {
539+
proc.removeAllListeners();
540+
} catch {}
541+
this.logGitCommandComplete(gitCommand, exception, getDurationMilliseconds(start), false, streamId);
542+
};
543+
527544
try {
545+
this.logGitCommandStart(gitCommand, streamId);
546+
528547
try {
529548
if (proc.stdout) {
530549
proc.stdout.setEncoding('utf8');
@@ -544,11 +563,14 @@ export class Git implements Disposable {
544563
exception = ex;
545564
throw ex;
546565
} finally {
547-
disposable?.dispose();
548-
proc.removeAllListeners();
549-
550-
this.logGitCommand(gitCommand, exception, getDurationMilliseconds(start), false);
566+
cleanup();
551567
}
568+
569+
// Ensure cleanup happens immediately when the generator is explicitly closed (e.g., via break or return)
570+
// This is called by JavaScript when the generator is abandoned, ensuring logGitCommand is called
571+
// synchronously rather than waiting for garbage collection.
572+
// eslint-disable-next-line @typescript-eslint/no-meaningless-void-operator
573+
return void cleanup();
552574
}
553575

554576
private _gitLocation: GitLocation | undefined;
@@ -1685,14 +1707,25 @@ export class Git implements Disposable {
16851707
terminal.sendText(text, options?.execute ?? false);
16861708
}
16871709

1688-
private logGitCommand(command: string, ex: Error | undefined, duration: number, waiting: boolean): void {
1710+
private logGitCommandStart(command: string, id: number): void {
1711+
Logger.log(`${getLoggableScopeBlockOverride(`GIT:→${id}`)} ${command} ${GlyphChars.Dot} starting...`);
1712+
this.logCore(`${getLoggableScopeBlockOverride(`→${id}`, '')} ${command} ${GlyphChars.Dot} starting...`);
1713+
}
1714+
1715+
private logGitCommandComplete(
1716+
command: string,
1717+
ex: Error | undefined,
1718+
duration: number,
1719+
waiting: boolean,
1720+
id?: number,
1721+
): void {
16891722
const slow = duration > slowCallWarningThreshold;
16901723
const status = slow && waiting ? ' (slow, waiting)' : waiting ? ' (waiting)' : slow ? ' (slow)' : '';
16911724

16921725
if (ex != null) {
16931726
Logger.error(
16941727
undefined,
1695-
`${getLoggableScopeBlockOverride('GIT')} ${command} ${GlyphChars.Dot} ${
1728+
`${getLoggableScopeBlockOverride(id ? `GIT:←${id}` : 'GIT')} ${command} ${GlyphChars.Dot} ${
16961729
isCancellationError(ex)
16971730
? 'cancelled'
16981731
: (ex.message || String(ex) || '')
@@ -1703,13 +1736,18 @@ export class Git implements Disposable {
17031736
);
17041737
} else if (slow) {
17051738
Logger.warn(
1706-
`${getLoggableScopeBlockOverride('GIT', `*${duration}ms`)} ${command} [*${duration}ms]${status}`,
1739+
`${getLoggableScopeBlockOverride(id ? `GIT:←${id}` : 'GIT', `*${duration}ms`)} ${command} [*${duration}ms]${status}`,
17071740
);
17081741
} else {
1709-
Logger.log(`${getLoggableScopeBlockOverride('GIT', `${duration}ms`)} ${command} [${duration}ms]${status}`);
1742+
Logger.log(
1743+
`${getLoggableScopeBlockOverride(id ? `GIT:←${id}` : 'GIT', `${duration}ms`)} ${command} [${duration}ms]${status}`,
1744+
);
17101745
}
17111746

1712-
this.logCore(`${getLoggableScopeBlockOverride(slow ? '*' : '', `${duration}ms`)} ${command}${status}`, ex);
1747+
this.logCore(
1748+
`${getLoggableScopeBlockOverride(`${id ? `←${id}` : ''}${slow ? '*' : ''}`, `${duration}ms`)} ${command}${status}`,
1749+
ex,
1750+
);
17131751
}
17141752

17151753
private _gitOutput: OutputChannel | undefined;

src/env/node/git/sub-providers/commits.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ import { wait } from '../../../../system/promise';
5858
import type { Cancellable } from '../../../../system/promiseCache';
5959
import { PromiseCache } from '../../../../system/promiseCache';
6060
import { maybeStopWatch } from '../../../../system/stopwatch';
61+
import { createDisposable } from '../../../../system/unifiedDisposable';
6162
import type { CachedLog, TrackedGitDocument } from '../../../../trackers/trackedDocument';
6263
import { GitDocumentState } from '../../../../trackers/trackedDocument';
6364
import type { Git, GitResult } from '../git';
@@ -1427,6 +1428,8 @@ async function parseCommits(
14271428
return { commits: commits, count: count, countStashChildCommits: countStashChildCommits };
14281429
}
14291430

1431+
using _streamDisposer = createDisposable(() => void resultOrStream.return?.(undefined));
1432+
14301433
if (stashes?.size) {
14311434
const allowFilteredFiles = searchFilters?.files ?? false;
14321435
const stashesOnly = searchFilters?.type === 'stash';

src/env/node/git/sub-providers/contributors.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { getLogScope } from '../../../../system/logger.scope';
1616
import { normalizePath } from '../../../../system/path';
1717
import type { Cancellable } from '../../../../system/promiseCache';
1818
import { PromiseCache } from '../../../../system/promiseCache';
19+
import { createDisposable } from '../../../../system/unifiedDisposable';
1920
import type { Git } from '../git';
2021
import { gitConfigsLog } from '../git';
2122
import type { LocalGitProvider } from '../localGitProvider';
@@ -91,6 +92,7 @@ export class ContributorsGitSubProvider implements GitContributorsSubProvider {
9192
'log',
9293
...args,
9394
);
95+
using _streamDisposer = createDisposable(() => void stream.return?.(undefined));
9496

9597
for await (const c of parser.parseAsync(stream)) {
9698
if (signal?.aborted || cancellation?.isCancellationRequested) {

src/env/node/git/sub-providers/graph.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ import { find, first, join, last } from '../../../../system/iterable';
5151
import { Logger } from '../../../../system/logger';
5252
import { getLogScope } from '../../../../system/logger.scope';
5353
import { getSettledValue } from '../../../../system/promise';
54-
import { mixinDisposable } from '../../../../system/unifiedDisposable';
54+
import { createDisposable, mixinDisposable } from '../../../../system/unifiedDisposable';
5555
import { serializeWebviewItemContext } from '../../../../system/webview';
5656
import type {
5757
GraphBranchContextValue,
@@ -181,6 +181,7 @@ export class GraphGitSubProvider implements GitGraphSubProvider {
181181
cursor?.skip ? `--skip=${cursor.skip}` : undefined,
182182
'--',
183183
);
184+
using _streamDisposer = createDisposable(() => void stream.return?.(undefined));
184185

185186
const rows: GitGraphRow[] = [];
186187

@@ -725,6 +726,7 @@ export class GraphGitSubProvider implements GitGraphSubProvider {
725726
'--',
726727
...files,
727728
);
729+
using _streamDisposer = createDisposable(() => void stream.return?.(undefined));
728730

729731
let count = 0;
730732
let hasMore = false;

src/env/node/git/sub-providers/status.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import { Logger } from '../../../../system/logger';
1919
import { getLogScope, setLogScopeExit } from '../../../../system/logger.scope';
2020
import { stripFolderGlob } from '../../../../system/path';
2121
import { iterateByDelimiter } from '../../../../system/string';
22+
import { createDisposable } from '../../../../system/unifiedDisposable';
2223
import type { Git } from '../git';
2324
import type { LocalGitProvider } from '../localGitProvider';
2425

@@ -226,6 +227,7 @@ export class StatusGitSubProvider implements GitStatusSubProvider {
226227
async hasConflictingFiles(repoPath: string, cancellation?: CancellationToken): Promise<boolean> {
227228
try {
228229
const stream = this.git.stream({ cwd: repoPath, cancellation: cancellation }, 'ls-files', '--unmerged');
230+
using _streamDisposer = createDisposable(() => void stream.return?.(undefined));
229231

230232
// Early exit on first chunk - breaking causes SIGPIPE, killing git process
231233
for await (const _chunk of stream) {
@@ -282,6 +284,7 @@ export class StatusGitSubProvider implements GitStatusSubProvider {
282284
'--others',
283285
'--exclude-standard',
284286
);
287+
using _streamDisposer = createDisposable(() => void stream.return?.(undefined));
285288

286289
// Early exit on first chunk - breaking causes SIGPIPE, killing git process
287290
for await (const _chunk of stream) {

0 commit comments

Comments
 (0)