Uploaded image for project: 'SLF4J'
  1. SLF4J
  2. SLF4J-557

MDCCloseable: not a great fit for a try-with-resources statement

    XMLWordPrintable

Details

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Major Major
    • 2.0.17
    • None
    • Core API

    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.

      try (MDCCloseable ignored = MDC.putCloseable("k", "v")) {
        // body
      } catch (Exception e) {
        log.error("An error occurred", e);
      }

      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:

      MDCCloseable mdcCloseable = MDC.putCloseable("k", "v");
      try {
        // body
      } catch (Exception e) {
        log.error("An error occurred", e);
      } finally {
        mdcCloseable.close();
      }

      But, as it turns out, this is not the case.

      As it's well explained here, 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:

      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
      }

      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`.

      Attachments

        Activity

          People

            ceki Ceki Gülcü
            alb-i986 Alberto Scotto
            Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated: