BIND 10 #275: refactoring CC session class for easier tests

[BIND 10 Development]

#275: refactoring CC session class for easier tests
----------------------------------------+-----------------------------------
 Reporter:  jinmei                      |        Owner:  jinmei                     
     Type:  enhancement                 |       Status:  reviewing                  
 Priority:  major                       |    Milestone:  06. 4th Incremental Release
Component:  Inter-module communication  |   Resolution:                             
 Keywords:                              |    Sensitive:  0                          
----------------------------------------+-----------------------------------

Comment(by jinmei):

 Replying to [comment:2 jelte]:
 >
 > The header file documentation was apparently already lagging behind the
 actual api, but now even more so :) (especially some information in
 ccsession.h about how the session should be passed, and what a caller
 would have to do, or that the caller would not be expected to do anything,
 which seems to be the case, since establish() is called in the constructor
 of CCSession).

 I've updated the description of the constructor.  But now that I've read
 the comment, I wonder it would be more natural if the caller must
 establish the session.  As a related matter, maybe we should explicitly
 detect and reject an attempt of doubly establishing a session? (I guess
 the underlying ASIO implementation detects it, but we should probably
 catch the invalid usage at a higher level).

 > For that matter, should we think about having another one where the user
 doesn't have to pass a Session, and let the ModuleCCSession take care of
 it, in case the caller really doesn't care?

 I'm not so sure about that.  To allow this, we should still pass an
 io_service (or our own wrapper object to ASIO) to the ModuleCCSession
 constructor or have ModuleCCSession construct an io_service internally.
 The former wouldn't be so convenient compared to the current constructor;
 the latter wouldn't be practical very much because in many cases objects
 like io_service would have to be managed in the top level application.

 In any case, we don't need this variation right now, so I'd suggest
 leaving the decision open for now.

 > For the sake of consistency, while not exactly essential, should we make
 *ccsession and *cs stack objects in auth's main.cc (and perhaps then
 rethink how we can do the same for auth_srv and io_service)?

 I'm not sure what this means.  Do you mean defining cc_session and cs in
 the anonymous namespace of auth/main.cc where auth_server and io_service
 are defined?  If so, I'd actually rather define the latter set more
 locally (because in general it's better to use a narrower scope for
 mutable variables).  But for now we need to define them in the namespace
 scope because they are referenced from free functions (my_config_handler
 and my_command_handler).  As a middle term plan we'll refactor this
 architecture with generalizing the ASIO link module, and then I think we
 can solve this in a cleaner way.  So I'd live with the inconsistency for
 now.

 > auth_srv.h declares a setSession() now, but it doesn't actually seem to
 be defined anywhere (or used, obviously :)) I suspect that this is a
 leftover and should be removed.

 Ah, good catch.  Actually, it's not a leftover in its usual sense.  It
 will be used in Trac #221, but not necessary in this branch.  I removed
 it.

 Is there anything I need to address further, or is it okay to merge it
 trunk?

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