BIND 10 #1966: logger formatter does too much in the destructor

[BIND 10 Development]

#1966: logger formatter does too much in the destructor
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:
  jinmei                             |                Status:  new
                       Type:         |             Milestone:  Next-Sprint-
  defect                             |  Proposed
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  logging                            |  Estimated Difficulty:  0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Description changed by jinmei:

Old description:

> While working on #1964, I noticed the destructor of the log::Formater
> class had quite a lot of responsibility.  In the context of #1964, the
> check mode can throw an exception, which is obviously bad.  Even with
> forgetting it, the destructor calls Logger::output(), which internally
> calls log4cplus modules that we cannot directly control and cannot be
> really sure if it's really safe.
>
> Although I agree it's a neat trick to let the destructor finish the
> output, it seems to me quite fragile to rely on at such a sensitive
> place as a destructor.
>
> I propose an alternative approach, which should be much safer in this
> regard, and hopefully will not damage the convenience of the current
> notation too much:
>
> - introduce another method to `Formatter`, say, output().  It
>   calls logger_->output().  In the check mode, it also performs the
>   "excess placeholder" check, and throws an exception if it finds an
>   error.  It records the fact it's called so that it cannot be called
>   more than once (if broken, throw).
> - (not absolutely necessary but for convenience) also introduce
>   another additional method, say, endarg().  It's similar to arg(),
>   but it also calls the newly introduced output().
> - the `Formatter` constructor now only does simpler and safer cleanup,
>   such as deleting the message (btw, it seems to be possible that the
>   message can leak depending on how the formatter is constructed.  We
>   should also fix that).  Also, in the check mode, it checks whether
>   print() has been called (or an exception has been thrown due to the
>   format check error), and if not, abort the program by assert()
>   (note that we cannot throw here).
> - The caller is responsible for completing the logging statements with
>   either endarg() or explicit print().  If broken, we'll lose that log
>   message in the normal mode, and we should be able to detect that via
>   an automated test that uses the check mode.
>
> A bonus of this approach is that we can now use exception for the
> "excess placeholder" check, which was changed to assert() in #1964
> as an urgent care regression fix.

New description:

 While working on #1964, I noticed the destructor of the log::Formater
 class had quite a lot of responsibility.  In the context of #1964, the
 check mode can throw an exception, which is obviously bad.  Even with
 forgetting it, the destructor calls Logger::output(), which internally
 calls log4cplus modules that we cannot directly control and cannot be
 really sure if it's really safe.

 Although I agree it's a neat trick to let the destructor finish the
 output, it seems to me quite fragile to rely on at such a sensitive
 place as a destructor.

 I propose an alternative approach, which should be much safer in this
 regard, and hopefully will not damage the convenience of the current
 notation too much:

 - introduce another method to `Formatter`, say, output().  It
   calls logger_->output().  In the check mode, it also performs the
   "excess placeholder" check, and throws an exception if it finds an
   error.  It records the fact it's called so that it cannot be called
   more than once (if broken, throw).
 - (not absolutely necessary but for convenience) also introduce
   another additional method, say, endarg().  It's similar to arg(),
   but it also calls the newly introduced output().
 - the `Formatter` constructor now only does simpler and safer cleanup,
   such as deleting the message (btw, it seems to be possible that the
   message can leak depending on how the formatter is constructed.  We
   should also fix that).  Also, in the check mode, it checks whether
   output() has been called (or an exception has been thrown due to the
   format check error), and if not, abort the program by assert()
   (note that we cannot throw here).
 - The caller is responsible for completing the logging statements with
   either endarg() or explicit output().  If broken, we'll lose that log
   message in the normal mode, and we should be able to detect that via
   an automated test that uses the check mode.

 A bonus of this approach is that we can now use exception for the
 "excess placeholder" check, which was changed to assert() in #1964
 as an urgent care regression fix.

--

-- 
Ticket URL: <http://bind10.isc.org/ticket/1966#comment:2>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development