Skip to content

demo(08-undisposed-httpclient): 08 — HttpClient field never disposed#157

Open
EricCogen wants to merge 1 commit intomainfrom
demo/08-undisposed-httpclient
Open

demo(08-undisposed-httpclient): 08 — HttpClient field never disposed#157
EricCogen wants to merge 1 commit intomainfrom
demo/08-undisposed-httpclient

Conversation

@EricCogen
Copy link
Copy Markdown
Owner

08 — HttpClient field never disposed

Expected verdict: ❌ Fails — GauntletCI should fire GCI0024 (resource lifecycle).

What changed

A new StripeWebhookClient class holds a HttpClient as a private
field that is constructed inline and never disposed:

public sealed class StripeWebhookClient
{
    private readonly HttpClient _http = new HttpClient();

    public Task<HttpResponseMessage> AcknowledgeAsync(Uri callback, CancellationToken ct)
        => _http.PostAsync(callback, content: null, ct);
}

The class itself does not implement IDisposable, so the underlying
socket handle outlives every request that uses it.

Why this is risky

  • HttpClient implements IDisposable. Owning one as a field without
    also implementing IDisposable (or routing through IHttpClientFactory)
    leaks the socket pool slot for the lifetime of the process.
  • Without a using/Dispose() pair anywhere in scope, exceptions thrown
    before the next request also leak any pending response stream.
  • The instance is shared across all callers of StripeWebhookClient, so
    a single misbehaving callback can starve the whole socket pool.

What GauntletCI catches

GCI0024 Resource Lifecyclenew HttpClient( allocated as a field
with no using, no surrounding Dispose() call, and no nearby
IHttpClientFactory reference.

Known co-fire: GauntletCI's GCI0039 External Service Safety rule
also flags the bare new HttpClient( instantiation (and the missing
explicit Timeout). Both rules describe the same root cause from
different angles; the lifecycle finding is the headline issue this
scenario is built to demonstrate.

See scenarios/08-undisposed-httpclient/README.md for the expected verdict.
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GauntletCI found issues in this PR. See inline comments for details.


public sealed class StripeWebhookClient
{
private readonly HttpClient _http = new HttpClient();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCI0024 — Resource Lifecycle

HttpClient allocated without using statement in src/OrderService/Payments/StripeWebhookClient.cs.

Evidence:

Line 5: private readonly HttpClient _http = new HttpClient();

⚠️ Why it matters: HttpClient implements IDisposable. Without using, it leaks OS handles or connection pool slots under exceptions.

💡 Suggested action: Wrap in using var resource = new HttpClient(...); to guarantee disposal.

Confidence: High | Severity: Warn


public sealed class StripeWebhookClient
{
private readonly HttpClient _http = new HttpClient();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCI0038 — Dependency Injection Safety

Direct instantiation of injectable type

Evidence:

src/OrderService/Payments/StripeWebhookClient.cs line 5: private readonly HttpClient _http = new HttpClient();

⚠️ Why it matters: Directly instantiating services bypasses the DI container, making the dependency untestable and unswappable.

💡 Suggested action: Register the type with the DI container and inject it via constructor.

Confidence: Low | Severity: Warn


public sealed class StripeWebhookClient
{
private readonly HttpClient _http = new HttpClient();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCI0039 — External Service Safety

Direct HttpClient instantiation

Evidence:

Line 5: private readonly HttpClient _http = new HttpClient();
File src/OrderService/Payments/StripeWebhookClient.cs adds HttpClient usage with no timeout configuration.

⚠️ Why it matters: Directly instantiating HttpClient bypasses the socket pool managed by IHttpClientFactory, causing socket exhaustion under load.

💡 Suggested action: Use IHttpClientFactory.CreateClient() or typed clients registered in the DI container.

Confidence: High | Severity: Block | 2 occurrences

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ec3825ad2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


public sealed class StripeWebhookClient
{
private readonly HttpClient _http = new HttpClient();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Dispose owned HttpClient or inject one from a factory

StripeWebhookClient allocates a HttpClient instance as a private field but never disposes it, and the class itself is not IDisposable. If this type is created repeatedly (for example, transiently in request scope), each instance leaves its handler/socket resources alive longer than intended, which can lead to connection exhaustion under load. Prefer injecting HttpClient (or IHttpClientFactory) or implementing disposal for the owned client.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant