BIND 10 #2903: need more asio error handling in asiodns

[BIND 10 Development]

#2903: need more asio error handling in asiodns
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  jinmei
            Priority:  medium        |                       Status:
           Component:  b10-auth      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130514
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:13 vorner]:

 > I'm not strictly against unrelated cleanups, but this amount seems like
 50% of the branch. If already written, I don't think it is worth throwing
 it out, or trying to rebuild the history for two branches, but the history
 would be cleaner if the works were separated. So, include it now, but
 let's be more careful about the amount of unrelated work in future.
 Anyway, I think there might be something wrong with out work process, if
 we can't push these cleanups in some fast way without cluttering other
 branches.

 I'm sorry for the excessive amount of changes.  I admit I tend to be a
 bad piggy-backer.  For this particular case, in retrospect I should at
 least have limited the changes to sync_udp_server internal.  As a
 general workflow, it's probably wise to not do any side work in
 general, or at least discuss it at the beginning of the review
 process, before showing the changes.

 > Also, a test in distcheck is failing for me. I don't know if it might be
 related, though. Should I try to find the cause of it?
 > {{{
 > ...F.......
 > ======================================================================
 > FAIL: test_bad_data (__main__.TestUserMgr)
 > ----------------------------------------------------------------------
 > Traceback (most recent call last):
 >   File
 "/var/tmp/bind10/bind10-2/bind10-20130221/_build/../src/bin/usermgr/tests/b10
 -cmdctl-usermgr_test.py", line 477, in test_bad_data
 >     'add', 'user1', 'pass1'
 >   File
 "/var/tmp/bind10/bind10-2/bind10-20130221/_build/../src/bin/usermgr/tests/b10
 -cmdctl-usermgr_test.py", line 141, in run_check
 >     self.assertEqual(expected_stdout, stdout.decode())
 > AssertionError: 'Using accounts file: test_users.csv\nError parsing csv
 file: newline inside str [truncated]... != 'Using accounts file:
 test_users.csv\n'
 >   Using accounts file: test_users.csv
 > - Error parsing csv file: newline inside string
 > }}}

 I have no idea about this one, and I couldn't reproduce it myself.
 It also looks quite unrelated to the changes in this branch...does
 that happen if you run the distcheck on a clean tree (after distclean,
 or even on a re-cloned repository)?

 > Can you explain in which cases this can throw? Allocation errors?
 Something else? If so, would it be some internal asio error?
 > {{{#!diff
 > -    io_.post(*this);
 > +    io_.post(*this);  // this can throw, but can be considered fatal.
 > }}}

 According to the ASIO documentation
 http://think-
 async.com/Asio/asio-1.4.8/doc/asio/reference/CompletionHandler.html
 it can throw at least for memory allocation failure, which should be
 okay to be considered fatal.  I have to admit I've not fully checked
 other possible reasons, but I believe it should be quite safe to say
 it wouldn't be triggered by external message (at least not directly),
 so I think it okay to let any possible exceptions be propagated.

 > Many `YIELD return` statements are replaced just by `return`? Is it
 safe? Was it YIELD really unneeded? Any idea why was it there before?

 If I understand it correctly, CORO_YIELD only makes sense if the
 function can be called again.  So, in the context of ASIO event
 handler, it should simply be no-op in effect unless it's followed by
 something that registers itself as the handler of new event, like
 async_xxx.

 I don't know exactly why there were patterns of `CORO_YIELD return`,
 but I suspect it began with a simple typo (redundant, though
 harmless), and has been naively copy-pasted.

 > Also, in many error cases, should we log? At least DEBUG?

 Hmm, maybe we should.  In some cases I chose ERROR as I believe they
 are really rare.

 > What was the use of `checkin_callback`? Is it used anywhere? Why the
 other server classes need it and the sync one does not? Should there be
 more explanation in the doxygen, than just „unused“?

 I created a ticket (#2935), also trying to answer these questions, and
 referred to the ticket from the documentation of `SyncUDPServer`.

 > Are these really unused? If so, why not remove them completely? Or do I
 understand the comment wrong?
 > {{{#!diff
 > -    // Objects to hold the query message and the answer
 > +    // Objects to hold the query message and the answer: these are not
 used
 >      isc::dns::MessagePtr query_, answer_;
 > }}}

 Ah, actually, `query_` is used.  I've updated the comment with more
 explanation on `answer_`.

 > In the last commit, you use object variable instead of local one. But,
 as all the local occurrences in other servers are `ec`, should this one be
 `ec_` instead of `ecode_`, for consistency?

 Actually, I somewhat intentionally avoid `ec_` because the
 `operator()` callback also has a parameter named `ec` and I thought
 one could confuse the other.  But it's not a strong opinion or
 preference (and as long as one is defined as a const reference it's
 less likely to be misused), so I changed it.

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