BIND 10 #2916: define asiolink::IOSocketLocal

BIND 10 Development do-not-reply at isc.org
Wed May 15 18:36:07 UTC 2013


#2916: define asiolink::IOSocketLocal
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130528
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:9 vorner]:

 > You set the `alarm` only sometimes. That's OK. But then, it should be
 reset with `alarm(0)` in the test tear down, so the alarm doesn't fire for
 some completely unrelated test later on, because that one takes too long.

 Good point, fixed.

 > Also, I know you set the original handler in the test destructor, so it
 doesn't matter, but I still believe it would be cleaner to reset the
 `g_io_service` to NULL as well, because afterwards the variable points to
 something invalid.

 okay, done.

 > I don't think this will prevent the possible deadlock here:
 > {{{#!c++
 >     alarm(IO_TIMEOUT);
 >     EXPECT_EQ(data_len, write(sock_pair_[1].get(), &expected_data[0],
 >                               data_len));
 > }}}
 >
 > In the theoretical case when write would block with too much data to
 write (and nothing would read on the other side). Then the alarm happens,
 interrupting the write (so it generates failure in the `EXPECT_EQ`), and
 calling `ioservice.stop()`. However, the service is started only *after*
 and it will wait forever for the data that'll never arrive.
 >
 > May I suggest leaving the abort to the default (killing the
 application), set it at each start of the test and abort on completion?

 This should actually be safe, but on thinking about it maybe it's
 safer to stop the test if write() fails.  I've updated the test that
 way with some detailed comments.

 > What is the purpose here?
 > {{{#!c++
 >     aux_sockpair[0].set(socks[0]);
 >     aux_sockpair[1].set(socks[1]);
 >     LocalSocket aux_sock(io_service_, aux_sockpair[0].release());
 > }}}
 >
 > If set never throws (it seemed to never throw), then there may be no
 exception between the set and release. If it can, then we can lose
 `socks[1]`.

 Ah, there's a slight bug here.  `LocalSocket` could throw (though we
 don't expect it here), and in that case we'd lose socks[0] (I guess
 you meant 0, not 1).  I've updated the code so it's more correct.

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


More information about the bind10-tickets mailing list