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

[BIND 10 Development]

#1522: Implementatio of isc::server_common::SocketRequestor
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  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      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:11 jinmei]:
 > 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.

 Well, I'm not sure in this case. I understand the class as kind of
 namespace/way to say this belongs together. This feels like if the guts
 are lying all over the place. But I don't really care about it much, it's
 aesthetic matter.

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

 Yes, I agree with this as well.

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

 Ah, the sendmsg contains a one-byte data. I didn't know that, so it might
 work.

 Anyway, it would be bad to close the connection. That would signal the
 boss to reclaim all the sockets transferred previously.

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

 ACK, I agree, it tests only stuff that happens after the sending part went
 berserk anyway.

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

 Hmm, what do you mean? Like if the boss sent a "2\n" and the socket? Yes,
 then it would leak, but can we really avoid that in any way?

 > - 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'd like to point that such situation shouldn't happen in practice and
 would be same kind of bug as a wrong data sent, etc. In the time the
 client picks up the socket, it was already prepared and reserved. It can
 be unavailable only in case the token is wrong.

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

 The newline is there to preserve some kind of consistency, and I wasn't
 sure if python wouldn't want to do some kind of line based buffering
 (well, it probably wouldn't).

 Anyway, the code as is looks OK, but I noticed one more problem I
 overlooked before. I don't see the code sending the token over the unix
 domain socket to tell the boss which socket is being requested.

 Thanks

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