Search code examples
c#try-catchserilogusingidisposable

C# .NET Core - Will a disposable persist across a try/catch block when declared outside? Is this poor practice?


Trying to understand best practice here with Serilog context, using statements, and some try/catch blocks. I'm trying to include the Id of some data in the Serilog context when I'm logging anything inside a handful of methods.

All of these methods are surrounded by simple try/catch blocks, and I'm trying to figure out the best way to insert my using statement.

This first method feels right to me, as I know it'll be disposed at the end of the method - but if something errors inside the try block, will it still dispose my ctxId object? My gut says no, but I can't seem to find an answer.

[Route("method")]
public async Task<IActionResult> Method(Parameters params) { 
  using var ctxId = Serilog.Context.LogContext.PushProperty("Id", params.Id.ToString());
  try { 
    // do stuff
    _logger.LogInformation("info...");
  }
  catch (Exception ex) {
    _logger.LogError("error...", ex); 
  }
}

Ideally, I don't want to have to do something like this (below) and duplicate these lines.

[Route("method")]
public async Task<IActionResult> Method(Parameters params) { 
  try { 
    // do stuff
    using var ctxId = Serilog.Context.LogContext.PushProperty("Id", params.Id.ToString());
    _logger.LogInformation("info...");
  }
  catch (Exception ex) {
    using var ctxId = Serilog.Context.LogContext.PushProperty("Id", params.Id.ToString());
    _logger.LogError("error...", ex); 
  }
}


Solution

  • but if something errors inside the try block, will it still dispose my ctxId object?

    Yes. using is still just "syntactic suggar" for try/finally. So your method be rewritten to something like:

    ContextId ctxId = null;
    try{
      ctxId = Serilog.Context.LogContext.PushProperty("Id", params.Id.ToString());
      try { 
        // do stuff
        _logger.LogInformation("info...");
      }
      catch (Exception ex) {
        _logger.LogError("error...", ex); 
      }
    }
    finally{
        ctxId?.Dispose();
    }
    

    No matter what happens* in the //do stuff, the ctxId?.Dispose() will still run. In general, best practice would be to reduce the scope of variables as much as possible. (Within reason, using to fine scoping can also hurt readability). If that is the entire method, as in your case, then that is perfectly fine.

    [ * ] There are some exceptions, like StackOverflowException, that cannot be caught. At that point you have to rely on the OS to do any cleanup.