[slf4j-dev] [JIRA] Updates for SLF4J-557: MDCCloseable: not a great fit for a try-with-resources statement

[QOS.CH (JIRA)]

SLF4J / SLF4J-557 [Open]
MDCCloseable: not a great fit for a try-with-resources statement

==============================

Here's what changed in this issue in the last few minutes.
This issue has been created
This issue is now assigned to you.

View or comment on issue using this link
https://jira.qos.ch/browse/SLF4J-557

==============================
 Issue created
------------------------------

Alberto Scotto created this issue on 02/Sep/22 11:05 PM
Summary:              MDCCloseable: not a great fit for a try-with-resources statement
Issue Type:           Bug
Assignee:             SLF4J developers list
Components:           Core API
Created:              02/Sep/22 11:05 PM
Labels:               MDC
Priority:             Major
Reporter:             Alberto Scotto
Description:
  Although the idea of the MDCCloseable is really nice, because it makes for some quite neat code (and I like 'neat' a lot), unfortunately there are some design drawbacks which make it a bad idea.
  
  Let me start with an example.
  {code:java}
  try (MDCCloseable ignored = MDC.putCloseable("k", "v")) {
    // body
  } catch (Exception e) {
    log.error("An error occurred", e);
  }{code}
  If an exception is thrown, would you expect the log statement to include the (k ,v) pair in the MDC or not?
   Or, put in another way, would you _like_ the log statement to include the (k ,v) pair in the MDC?
  
  I definitely would.
   The reason is that in general I would expect a try-with-resources statement to execute the close() as the very last thing, _after_ the catch block was executed.
  
  In other words, I would expect a try-with-resources statement to behave just like a plain old try/catch/finally statement:
  {code:java}
  MDCCloseable mdcCloseable = MDC.putCloseable("k", "v");
  try {
    // body
  } catch (Exception e) {
    log.error("An error occurred", e);
  } finally {
    mdcCloseable.close();
  }{code}
  But, as it turns out, this is not the case.
  
  As it's well explained [here|https://stackoverflow.com/questions/32849066/java-try-with-resources-vs-try-catch-finally-about-order-of-autoclosing], when a try-with-resources statement has a catch block, the catch block is executed _before_ the implicit finally, i.e. the close().
  
  While for a resource like a handle to a file or a socket it's irrelevant whether the resource is closed before or after the catch block, for a MDCCloseable it does matter a lot.
  
  Because, as we can see in the example, if we have a log statement in the catch block, and the MDCCloseable defined an important piece of information (say the ID of an entity which was being processed in the try block), closing the resource before the catch block means that  our log statement will be missing that piece of information.
  
  So every time we need to define a catch block along with the try-with-resources holding our MDCCloseable, we actually need to throw away the try-with-resources, and write a plain old try/catch/finally:
  {code:java}
  MDCCloseable mdcCloseable = MDC.putCloseable("k", "v")
  try {
    // body 
  } catch (Exception e) {
    log.error("An error occurred", e);
  } finally {
    mdcCloseable.close(); // in this form, if body throws an exception, close() is executed _after_ the log statement
  }{code}
  What about those try-with-resources statements where we don't have a catch block?
   Well _at the minute_ we don't, but we might in the future, and if that happens there's a chance we might forget to switch to a plain old try/catch/finally, therefore missing some pieces of information in the logs.
  
  For this reason I would like to ask to throw away the MDCCloseable completely, by starting deprecating the method `MDC#putCloseable`.


==============================
 This message was sent by Atlassian Jira (v8.8.0#808000-sha1:e2c7e59)