BIND 10 #3008: Develop NameChange{Sender,Listener} classes.

[BIND 10 Development]

#3008: Develop NameChange{Sender,Listener} classes.
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130731
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:  n
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by tmark):

 >
 > DHCP_DDNS_NCR_SEND: a part of the error message goes to the new line
 which (as far as I understand) will cause truncation of the error message.
 That is because the second line of the message definition starts the
 internal description.
 >

 Fixed.

 > Also there are other error messages which names do not tell that they
 are errors:
 > - DHCP_DDNS_NCR_RECV_NEXT
 > - DHCP_DDNS_NCR_SEND_NEXT
 > Please consider appending ERROR to them to.

 Fixed.

 >
 > >
 > >
 > > > '''src/bin/d2/ncr_io.cc'''
 > > > '''!NameChangeListener Constructor''': invalid alignment of the
 second line of parameters of ''isc_throw''.
 > > >
 > > > Neat pick: suggest use of !''constructor!'' instead of ctor in the
 exception string.
 > > >
 > >
 > > Replaced ctor with "constructor" and re-aligned.
 > >
 > > > '''NameChangeListener::stopListening''': I am wondering if you
 should call
 > > > {{{
 > > > setListening(false)
 > > > }}}
 > > > if exception has occured during invocation of ''close()''. It looks
 to me that exception is a sign of error that causes the listener to
 continue to run (because it couldn't be closed).
 > > >
 > >
 > > My intent is that it allow an attempt to re-open, even if the close
 failed, which is why it swallows the exception now.  If the state does not
 change, then a
 > > subsequent call to startListening will fail, as the state is already
 > > "listening".  This would mean that the listener could not attempt to
 re-open
 > > its IO source and cannot recover on its own.  By always changing
 marking listening false then at least the attempt to recover can be made.
 I have added some
 > > commentary to explain this.
 >
 > Fair enough. I would probably introduce some additional state, e.g.
 recovery etc. But it is ok like it is now.
 >
 > >
 > >
 > > > '''NameChangeListener::invokeRecvHandler''': I don't understand why
 recv_handler_ must be declared const here and thus why const_cast is
 used?. If you want to make sure not to modify he handler, you could
 provide an accessor function and use it to access the pointer. However, I
 am still not sure if it is required here.
 > > >
 > >
 > > What I should have done was just declare recv_handler_ as a reference
 which is
 > > what I have done.  I did the same for send_handler_ in
 NameChangeSender.
 >
 > That may work.
 >
 >
 > >
 > > >
 > > > '''NameChangeSender::invokeSendHandler''':
 > > >
 > > > What happens if the Completion Handler throws an exception? It is
 not handled here and will be propagated down to the thread where
 io_service.run() was called. The !''sendNext!'' function will not be
 called.
 > > >
 > > > The same issue applies to NameChangeListener::invokeSendHandler
 > >
 > > Funny you should ask. I came across this while working on the queue
 manager,
 > > which is the next layer up from NameChangeListener. So I added the
 following
 > > note in the ncr_io.h to the method description invokeRecvHandler (and
 > > similarly worded note above invokeSendHandler:
 > >
 > > {{{
 > >     /// The handler invoked by this method MUST NOT THROW. The handler
 is
 > >     /// application level logic and should trap and handle any errors
 at
 > >     /// that level, rather than throw exceptions.  If IO errors have
 occurred
 > >     /// prior to invoking the handler, they are expressed in terms a
 failed
 > >     /// result being passed to the handler.  Therefore any exceptions
 at the
 > >     /// handler level are application issues and should be dealt with
 at that
 > >     /// level.
 > >     ///
 > >     /// If the handler were to throw, the exception will surface at
 > >     /// IOService::run (or run variant) method invocation as this
 occurs as
 > >     /// part of the callback chain.  This will cause the invocation of
 > >     /// doReceive to be skipped which will break the listen-receive-
 listen
 > >     /// cycle. To restart the cycle it would be necessary to call
 > >     /// stopListener() and then startListener().
 > > }}}
 > >
 >
 > The !''invokeSendHandler!'' method provides a mechanism to communicate
 an error via result. Why not be on the safe side and catch exceptions from
 the user's callback and set appropriate result. That way you would just
 have a single point where you catch errors.
 > If you don't agree, the comment is fine.

 I have wrapped the handler invocations within try-catch blocks as fail-
 safes and have amended the comments accordingly. The handler is
 still mandated to catch and handle all exceptions and failure to do so is
 considered a violation of the interface.

 >
 > >
 > > >
 > > > '''!NameChangeListener Constructor''': Are there any use cases when
 recv_handler could be NULL, indicating that no callback should be called
 on request completion? If not, throwning exception when recv_handler is
 NULL is fine.
 > >
 > >
 > > No, I cannot see a case for this.  Listeners and Senders are of much
 value
 > > on their own.  The handlers are the only mechanism they supply for
 exposing
 > > the results of their IO operations.   Also, since I have replaced the
 const
 > > raw pointers with references then the caller has no choice but to
 supply
 > > a handler.
 >
 > Fair enough.
 >
 > >
 > > the only behavioral difference made.  Additionally, in the case of
 recovery, where the socket was open and was then closed and reopened, we
 could theoretically fail if there were late arriving packets.  I suppose
 there might be a case, where we actually do want to use it production.
 >
 > That's fine. But the comment in the function body is still misleading:
 > {{{
 >         // If in test mode, enable address reuse.
 > }}}
 > >

 I missed this before. Fixed.

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