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

[BIND 10 Development]

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

Comment (by marcin):

 Replying to [comment:7 tmark]:
 > Replying to [comment:6 marcin]:
 > > Reviewed commit a765638850588cc8980c622351124f1319c46365

 >
 > >
 > > General comment: I suggest that !''queue!'' or !''QUEUE!'' is used,
 instead of !''que!'' or !''QUE!'' throughout the code. I understand that
 !''que!'' is shorter and thus easier to type but when I think about the
 variable or function name that refers to a queue I would rather type the
 longer form and would be surprised to see that the symbol does not exist.
 Besides that, the longer form is more widely used in the code.
 > >
 >
 > Yes, I agree. Somewhere along the way I opted for "que" or "Que" it the
 code and "queue" or "Queue" in documentation.  It is now "queue" or
 "Queue" everywhere.
 >
 > > '''src/bin/d2/d2_messages.mes'''
 > > Suggest that the following messages: DHCP_DDNS_NCR_UDP_(SEND |
 LISTEN)_CLOSE is renamed to something like DHCP_DDNS_NCR_UDP_(SEND |
 LISTEN)_CLOSE_ERROR to reflect that it is an error. The other name
 suggests it is an informational message that something has been closed.
 > >
 >
 >
 > Added _ERROR.
 >
 >
 > > '''DHCP_DDNS_NCR_UDP_RECV_ERROR''': Looks like spurious !''in the
 request!''.
 > >
 >
 > Fixed.

 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.

 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.

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

 >
 > >
 > > '''!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.

 >
 > >
 > > '''src/bin/d2/ncr_udp.cc'''
 > > '''NameChangeListener::open''':  The comment in this section is
 misleading:
 > > {{{
 > >     // If in test mode, enable address reuse.
 > >     if (reuse_address_) {
 > >
 asio_socket_->set_option(asio::socket_base::reuse_address(true));
 > >     }
 > > }}}
 > > It is should be mentioned that reuse_address_ is only used by unit
 tests (if it is true). Perhaps, reuse_address_ (and reuse_address_ in the
 constructor) could be then renamed to test_mode_ (test_mode). At least it
 should be mentioned in the constructor that address reuse should be set to
 true in the unit tests only.
 >
 > Initially, I did call it test_mode, but then thought reuse_address is
 more explicit and it is
 > 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.
 }}}
 >
 >
 >

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