Review this exception-handling code from an order service. Identify every problem, explain why each one matters in production, and show how you would rewrite it.

Code under review

public class OrderService
{
    private static readonly SemaphoreSlim _lock = new(1, 1);

    public async void ProcessOrder(string json)                 // 1
    {
        try
        {
            _lock.Wait();                                       // 2
            var order = JsonSerializer.Deserialize<Order>(json);
            int qty;
            try { qty = int.Parse(order.Quantity); }            // 3
            catch { qty = 1; }

            var stock = _inventory.GetStockAsync(order.Sku).Result;   // 4
            if (stock < qty) throw new Exception("No stock");    // 5

            await _repo.SaveAsync(order);
            _lock.Release();                                    // 6
        }
        catch (Exception ex)
        {
            _logger.LogError("Order failed: " + ex.Message);    // 7
            throw ex;                                           // 8
        }
    }

    public Order GetOrder(int id)
    {
        try { return _repo.Get(id); }
        catch (Exception) { return null; }                      // 9
    }
}

How I would run this review

I'd group findings by severity, because in a real review the order you raise things matters: first what can stall or crash production, then what corrupts data or hides failures, then diagnostics, then design.

Severity 1 — can stall or crash the service

(6) _lock.Release() outside finally. Any exception between Wait and Release (bad JSON, bad quantity, out of stock, database timeout) leaves the semaphore at 0 forever. Because the semaphore is static, every subsequent order in the process waits indefinitely. The service looks healthy (process up, CPU idle) while the queue backs up — the worst kind of outage. Fix: acquire, then try { ... } finally { Release(); }.

(1) async void. Callers can't await it, can't know when it finished, and can't catch its exceptions. An exception escaping an async void method is rethrown on the thread pool and terminates the process (in ASP.NET Core and workers). Combined with throw ex; at the end, every failed order crashes the host. Fix: async Task (and async Task<Result> here).

(2) _lock.Wait() and (4) .Result in async code. Both block a thread-pool thread. Under load this causes thread-pool starvation: requests queue, latency spikes, timeouts cascade. .Result also risks deadlocks under a SynchronizationContext, and wraps failures in AggregateException, so specific catch blocks never match. Fix: await _lock.WaitAsync(ct) and await _inventory.GetStockAsync(sku, ct).

Severity 2 — wrong data or hidden failures

(3) Exceptions as control flow + silent default. int.Parse in a try/catch is slow for bad input, but the real bug is business logic: invalid quantity becomes 1 and the order is placed anyway. The customer and warehouse get something nobody asked for. Fix: int.TryParse and reject invalid input with a validation result (400).

(9) Catch-all returning null. A database outage, a timeout or a mapping bug now looks like "order not found". Callers may create duplicates, show "no order" to customers, or skip refunds. Fix: let infrastructure exceptions propagate; return null only for a genuine not-found from the repository.

Missing null check after Deserialize. JsonSerializer.Deserialize<Order>("null") returns null, so order.Quantity throws NullReferenceException; malformed JSON throws JsonException. Both are client errors (400), not server errors (500).

Severity 3 — diagnostics

(7) LogError("Order failed: " + ex.Message). Loses the stack trace and inner exceptions (the SqlException inside a DbUpdateException), breaks structured logging (no OrderId property, no template grouping), and may log sensitive data if messages contain payload values. Fix: logger.LogError(ex, "Order {OrderId} failed", order.Id) — and log once, at the boundary.

(8) throw ex;. Resets the stack trace to this line (CA2200), so the log and Application Insights point here rather than the real source. Fix: throw;, or better, remove this catch entirely and let the global handler log once.

Cancellation. No CancellationToken flows through, so when a client disconnects or the host shuts down, work continues and the lock is held longer. If cancellation were added, OperationCanceledException would currently be logged as an error.

Severity 4 — design

(5) throw new Exception("No stock"). Callers can't distinguish "out of stock" (an expected business outcome → 409/422, maybe a back-order) from a crash (→ 500). Fix: a result type (Result.Fail(OrderErrors.InsufficientStock(sku, requested, available))), or a specific InsufficientStockException with properties if your architecture maps domain exceptions globally.

A global lock around I/O. A process-wide semaphore serialises all orders, including slow database calls, and only works on one instance — with two replicas in Azure, it provides no protection at all. Real stock correctness needs a database-level mechanism: a conditional update (UPDATE ... SET Stock = Stock - @qty WHERE Sku = @sku AND Stock >= @qty), optimistic concurrency (rowversion), or a per-SKU lock if serialisation is required.

One method doing everything (parsing, validation, locking, inventory, persistence) makes error handling tangled. Split validation from the application service.

The rewrite

public sealed class OrderService(IInventory inventory, IOrderRepository repo, ILogger<OrderService> logger)
{
    public async Task<Result<OrderId>> ProcessOrderAsync(string json, CancellationToken ct)
    {
        OrderRequest? request;
        try
        {
            request = JsonSerializer.Deserialize<OrderRequest>(json, JsonOptions.Strict);
        }
        catch (JsonException ex)
        {
            logger.LogWarning(ex, "Rejected malformed order payload");      // no payload contents logged
            return OrderErrors.InvalidPayload;
        }

        if (request is null || !int.TryParse(request.Quantity, NumberStyles.None, CultureInfo.InvariantCulture, out int qty) || qty <= 0)
            return OrderErrors.InvalidQuantity;

        // Atomic, multi-instance-safe stock reservation in the database (no process-wide lock)
        StockReservation reservation = await inventory.TryReserveAsync(request.Sku, qty, ct);
        if (!reservation.Succeeded)
            return OrderErrors.InsufficientStock(request.Sku, qty, reservation.Available);

        OrderId id = await repo.SaveAsync(request.ToOrder(qty), ct);   // transient retries handled by a resilience pipeline
        return id;
        // No catch-log-rethrow: unexpected exceptions reach the global IExceptionHandler, which logs once
        // with the trace ID and returns a generic 500 ProblemDetails. Cancellation propagates.
    }

    public Task<Order?> FindOrderAsync(int id, CancellationToken ct) => repo.FindAsync(id, ct);   // null = not found only
}

If keeping an in-process lock really were required, the minimum fix is:

await Gate.WaitAsync(ct);
try { /* work */ }
finally { Gate.Release(); }

How I'd close the review

I'd raise the lock leak and async void as blocking comments with the incident risk explained, suggest the rest as required changes, and propose guardrails: analysers CA2200 (throw ex), CA2201 (throwing Exception), CA1031 (catching general exceptions), VSTHRD100 (async void), VSTHRD002/VSTHRD103 (sync waits), CA2016 (forward CancellationToken), CA2254/CA1848 (log templates), set to errors; a short exception-handling checklist in the PR template; and a health check based on queue progress, not just process liveness.

Production example

[In a real incident at my company] a semaphore released outside finally caused the order processor to stop after [the first malformed message]; health checks stayed green because the process was alive, and the only log line had no stack trace because of ex.Message logging and throw ex. The review checklist above came out of that post-incident review.