BIND 10 #2617: MSGQ_RECV_ERR errors on clean shutdown

[BIND 10 Development]

#2617: MSGQ_RECV_ERR errors on clean shutdown
-------------------------------------+-------------------------------------
            Reporter:  jreed         |                        Owner:
                Type:  defect        |  jinmei
            Priority:  medium        |                       Status:
           Component:  msgq          |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130205
         Sub-Project:  Core          |                   Resolution:
Estimated Difficulty:  Discuss (4?)  |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  Low
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:14 vorner]:

 > I have few small notes. First of, is `ECONNRESET` really harmless? It
 means the other side was either Windows (which couldn't have happened with
 a local-domain socket) or the other side died in a painful death without
 closing the socket. It also means they could miss reading all data in the
 socket. So, I'm not sure it should be handled the same as ordinary EOF.

 Hmm, based on this comment I realized I naively assumed ECONNRESET can
 be handled like 0 being returned from recv().  After trying to figure
 out how exactly that could happen, I found it quite difficult to
 reproduce with some simplified experiments.  According to logs from
 BIND 10, that generally seems to happen for connections with xfrin,
 xfrout, and cmdctl.  I suspected the use of threads may be related,
 but experiments with threads still didn't reproduce it.

 At this point I gave up figuring it out - I guess it's beyond the
 scope of this ticket.  Since we (I) don't know how that happens and
 (therefore) what kind of bad things can happen with it, it wouldn't be
 a good idea to pretend it cannot be serious.  So I decided to err on
 the side of caution by logging all socket.error at the error level.
 I clarified that it's probably less harmful if it happens on shutdown
 in the detailed version of log message, even though I know no one will
 read it.

 > Why if the socket is writable it is not readable? Couldn't it be written
 and read both at the same time?

 As I asked in my previous quick response, I'm not sure which part of
 the code you're referring to.  Maybe this one?
 {{{#!python
                         writable = event & select.POLLOUT
                         readable = not writable and (event &
 select.POLLIN)
 }}}
 if so, actually, I don't know:-) I wondered the same thing, but in
 this task I chose to preserve the original code logic:
 {{{#!python
                         if event & select.POLLOUT:
                             self.__process_write(fd)
                         elif event & select.POLLIN:
                             self.process_socket(fd)
 }}}
 we can probably loosen it, but I didn't want to break it accidentally
 as it's outside the scope of this task.

 > This exception:
 > {{{#!python
 > class MsgQCloseOnReceive(Exception):
 >     '''Exception raised when reading data from a socket results in
 "shutdown.
 >
 >     This can be either getting 0-length data or via ECONNRESET
 socket.error
 >     exception.  This class holds whether it happens in the middle of
 reading
 >     (i.e. after reading some) via partial_read parameter, which is set
 to True
 >     if and only if so.  This will be used by an upper layer cathing the
 >     exception to distinguish severity of the event.
 >
 >     "'''
 >     def __init__(self, reason, partial_read):
 >         self.partial_read = partial_read
 >         self.__reason = reason
 >     def __str__(self):
 >         return self.__reason
 > }}}
 >
 > I think the double-quotes in the docstring are somewhat confused. Also,
 I thought we usually put empty lines between method definitions.

 Hmm, it in fact looks like the convention is to not use double-quotes
 in docstrings from a quick browse of python standard libs.  Changed it
 to single-quote, and also fixed the incomplete quotation.  Also added
 an empty line.

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