BIND 10 #3008: Develop NameChange{Sender,Listener} classes.
BIND 10 Development
do-not-reply at isc.org
Tue Jul 23 15:37:34 UTC 2013
#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
-------------------------------------+-------------------------------------
Changes (by tmark):
* owner: tmark => marcin
Comment:
Replying to [comment:6 marcin]:
> Reviewed commit a765638850588cc8980c622351124f1319c46365
>
> In general, a piece of very good and useful code.
>
> '''Review comments'''
>
> This commit contains invalid email address:
> {{{
> commit a765638850588cc8980c622351124f1319c46365
> Author: Thomas Markwalder <tmark at debian-tkm-01.tmark>
> Date: Thu Jul 18 07:21:22 2013 -0400
>
> [3008] Removed extraneous const function return qualifiers the
debian
> complained about.
>
> }}}
>
> Perhaps committed from the VM.
Yes it was. I did not realize it included email addresses. Duly noted for
future reference.
>
> 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.
> '''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.
> '''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.
> '''!NameChangeSender Constructor''': Neat pick: Suggest that you replace
!''ctor!'' with !''constructor!''.
>
Done.
> '''NameChangeSender::startSending''': spurious space between
!''!IOService!'' and ampersand.
Oops.
> Also, there should be a space after !''Open failed:!''.
Yeah, yeah.... got it. Man are you picky. I feel bad for you new puppy.
He
will to work very hard to please you. ;)
>
> '''NameChangeSender::stopSending''': similar comment as previously for
!''NameChangeListener::stopListening!
> ''.
Same answer as above.
>
> '''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().
}}}
>
> '''src/bin/d2/ncr_io.h'''
> In the file description, one line runs on another.
Oops.
> Also, it would be more logical to me if the layers were described/listed
from the top level layer to bottom level layer. In other words, perhaps
you could swap IO Layer with !NameChangeRequest layer.
>
Ok, I re-ordered them.
> Duplicate !''implement!'' in !''The abstract classes defined...!''.
>
> '''!NcrListenerError class''': the description could be extended. For
example:
> {{{
> @brief Exception thrown if an error occurs other than covered by @c
NcrListenerOpenError and @c NcrListenerReceiveError.
> }}}
> or something like this.
>
It could be. I settled for slightly different wording.
> '''!RequestReceiveHandler''': Technically, the !NameChangeRequestPtr
could be passed by a reference.
>
> Neat pick: Result parameter could be declared const. But it doesn't have
to.
Changed handler signatures to const and reference as suggested.
>
> '''!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.
>
> '''invokeRecvHandler''': !NameChangeRequestPtr could be passed by
reference.
>
Done.
> '''RequestSendHandler::operator()''': NameChangeRequestPtr could be
passed by reference.
>
Done.
> '''!RequestSendHandler constructor''': are there any use cases when
callback could be NULL to indicate that no callback should be called upon
request completion?
>
See comment above pertaining to listener constructor.
> '''sendRequest''': !NameChangeRequestPtr could be passed by reference.
Done.
>
> '''invokeSendHandler''': argument could be const. It doesn't have to
though.
Done.
>
> '''flushSendQue''': the name starting with !''flush!''suggests that all
requests in the queue are dispatched while they are actually removed.
>
> '''getNcrToSend''': I am wondering if getNcrToSend should return a const
reference to a pointer or maybe it shoulnd rather return NCR (not a
pointer) by value. The reason is that when you return a reference it may
invalidate when ncr_to_send_ is replaced with the new NCR by the logic
processing the queue. As a result, the value held by the caller may get
changed after he has got it. Something like this could work:
> {{{
> NameChangeRequest getNcrToSend() const {
> return (*ncr_to_send_);
> }
> }}}
> Also, whatever you choose, this method should be const.
I ended up not using this method anywhere and in retrospect it really
isn't of value outside of
Additionally, really nothing outside of NameChangeSender can reliably use
this value, so I
removed the method altogether.
>
> '''src/bin/d2/ncr_udp.cc'''
> '''UDPCallback::operator()''': Neatpick: invalid alignment in the second
line of the parameters list.
>
> '''!NameChangeUDPListener ctor''': Please check the line breaks etc. in
the constructor against coding guidelines.
>
I rearranged it some.
> '''NameChangeUDPListener::open''': typo !''leve!'', instead of
!''level!''.
>
> '''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.
--
Ticket URL: <https://bind10.isc.org/ticket/3008#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list