BIND 10 #1522: Implementatio of isc::server_common::SocketRequestor

[BIND 10 Development]

#1522: Implementatio of isc::server_common::SocketRequestor
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jelte
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120110
  blocker                            |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  10
Feature Depending on Ticket:         |
  Socket creator                     |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:10 vorner]:

 As I reported on the daily call, I'm taking over the ticket so we can
 complete the socket creator tasks while Jelte is taking off.

 I've re-reviewed the entire branch as a successor implementor, and
 made some unrelated cleanups: from 80d9d4f to d8ac471.  Most of them
 should be trivial and non controversial, but there's one substantial
 set of changes: I moved some static/private member functions of
 SocketRequestor because they don't need any private information of the
 class.  I generally believe it's better to implement things outside of
 a class if they can reasonably be implemented only via the class's
 public interfaces.

 > > Hmm, looking at that new code I think there is more of it that is
 intended to be used here (or if not intended, at least useful).
 > >
 > > For now I've went for the path of least resistance, added a close() in
 the existing throws. This will still leak if there is any other exception
 in that part of the code (or if a new throw is added). But given the
 seeming intent of the classes in that socketsession, this functionality
 should probably be moved there as well (i.e. why not let
 SocketSessionReceiver create and connect to a socket, instead of passing
 it an opened one? or perhaps provide both options if there is a use-case
 for the existing one)
 >
 > I don't think anything else there could throw, as these are C calls.
 >
 > Anyway, would you add a ticket to incorporate the code from the
 socketsession, so we don't have some code twice?
 >
 > > Same as above; for now I left this in here, as is, with a TODO message
 (oh the pain, committing code with TODOs).

 On looking it further, I guess we should eventually merge the FD
 passing part of the socket creator interface and SocketSession
 framework (is this what Jelte meant by "functionality should be
 moved"?  If so, I tend to agree).  Both do quite the same things using
 pretty low level interfaces, which are generally more error prone and
 less portability.  So unifying such problematic part makes more sense
 than other general cases of reducing duplicates.  But that would be
 beyond the scope of the initial development of socket creator support.
 So I'm okay with deferring it to post release development cycle.

 > Still the asterisk on the wrong side of space ;-).

 I believe I fixed it and other similar style issues.

 > Replying to [comment:9 jelte]:
 > > >  * The status messages from boss are 2 bytes long, either "0\n" or
 "1\n", you seem to take only the one byte.
 > >
 > > ah, doh, changed.
 >
 > But you still receive (and test) messages that are "0\0" and "1\0", not
 "0\n" and "1\n" (that's a newline, not null-byte). You need to update the
 constants, the tests and the buffer for reading should be 3 bytes large,
 if you want to compare it with ==.

 This should be fixed in 182024b.

 > > >  * When you send a nonsense instead of send_fd (eg. when it is -2),
 can you be sure the socket will be in a consistent state? And that it'll
 eat the one wrong byte somehow? Because you seem to use the socket after
 that.
 > >
 > > Well, in a way, reusing it and seeing that the next one does not fail
 is a way of testing this. I couldn't really come up with any better way to
 test it :)
 >
 > Well, my reasoning is, if nothing guarantees this should be one byte,
 then there'll be system that'll fail (solaris, for example).
 >
 > And besides, if someone sends nonsense, then it might as well be other
 amount of nonsense and the protocol gets completely confused. Is there a
 reason for this in practice?

 I've not fully understood the issue here.  What happens in the -2 case
 is the child sends one byte data and it's received in recv_fd() where
 one the byte data is received by recvmsg() but since no FD is passed
 recv_fd() fails and requestSocket() throws.  I don't see how this
 makes the subsequent doRequest() fail...  That said, I think if
 requestSocket() fails due to an unexpected error, it generally cannot
 be assumed to be usable anymore and the caller application should
 basically terminate the connection (and reopen it if necessary).  The
 SocketSession framework is designed that way.

 If we agree, I'm okay with removing the "doRequest() after failure"
 test as the behavior in such a case wouldn't be guaranteed.

 I've noticed some other, higher design level things while I looked at
 the code more closely.  These may not have to be addressed in this
 release cycle but I'd like to clarify/revise it especially if we
 consider unifying this API/implementation with SocketSession:

 - In the current implementation (and probably as a result of the
   design) I suspect a socket was passed and could leak if "status" is
   bogus.  Admittedly this wouldn't normally happen, but such a design
   seems to be fragile to me.
 - I personally don't think it the best idea to signal the case of
   "CREATOR_SOCKET_UNAVAILABLE" by exception.  At the very least it
   should be specifically separated from other exceptions such as the
   one as a result of bogus status data; otherwise, assuming the app
   should generally drop the connection on the latter case, we could
   drop and re-establish the connection unnecessarily
 - I also don't think the interface using magic strings like "0\n" is
   clean.  As we even already noticed such string manipulation is error
   prone.  It's also counter intuitive to see the existence of '\n'
   (although this might be mitigated by introducing some constant
   representation).  Also, passing non atomic data like this before
   passing fd using sendmsg/recvmsg would cause some tricky failure
   modes such as the one mentioned in the first bullet above.  I'd
   rather use the currently-unused one-byte data that is passed with
   the FD using sendmsg/recvmsg.

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