Follow-up from Improve RADIUS unit tests
The following discussions from !474 (merged) should be addressed:
-
@andrei started a discussion: (+3 comments) I'm not sure about
if stopped restart
. It hides a possible ongoing run or poll that could have resulted in a crash. But then again, maybe we don't care about that, but rather about posted handlers. I don't have a preference. But now we have to worry about two polls on the same io service. Is that UB I wonder? -
@andrei started a discussion: (+2 comments) The point of adding the same test id twice to the thread pool was to check how a packet coming from the same client is processed concurrently. I think that is very important in MT. If I restore that code in access UTs
diff --git a/src/hooks/dhcp/radius/tests/access_unittests.cc b/src/hooks/dhcp/radius/tests/access_unittests.cc index 1d0091d4..ad8dedad 100644 --- a/src/hooks/dhcp/radius/tests/access_unittests.cc +++ b/src/hooks/dhcp/radius/tests/access_unittests.cc @@ -631,2 +631,3 @@ struct MTAccessTest : AccessTest { for (uint16_t i = 0; i < expected_received_; ++i) { + for (uint16_t j = 0; j < 2; ++j) { boost::shared_ptr<function<void()>> const work( @@ -634,2 +635,3 @@ struct MTAccessTest : AccessTest { EXPECT_NO_THROW_LOG(thread_pool_.add(work)); + } }
... then, the UTs that previously failed, fail in this branch as well, plus some of the ones you added:
[ FAILED ] 4 tests, listed below: [ FAILED ] MTAccessTest.twoQueries4 [ FAILED ] MTAccessTest.twoQueries6 [ FAILED ] MTAccessTest.noHost4 [ FAILED ] MTAccessTest.noHost6
I get concernig errors such as this:
access_unittests.cc:1402: Failure Expected: handle->getContext("subnet4", subneth) doesn't throw an exception. Actual: it throws isc::hooks::NoSuchCalloutContext with description "unable to find callout context associated with the current library index (-1)".
Other errors are less concerning and are probably fixed by expecting half of the packets to be dropped.
- EXPECT_EQ(0, steps_[CalloutHandle::NEXT_STEP_DROP]); + EXPECT_EQ(expected_received_ / 2, steps_[CalloutHandle::NEXT_STEP_DROP]);
It might be that the test itself needs changing. But shouldn't we look into it? You can leave it to me, if you're not willing to pursue this.
-
@andrei started a discussion: (+3 comments) I would also like to fix one behavior, and this issue seems like the last opportunity where we're going to be able to fix it before stable.
If I send DHCP traffic to a Kea server that has RADIUS configured, but no RADIUS server started. And I abruptly sigint Kea, it pauses for a while, and then I get a lot of ASAN reports. Not a big problem.
Here is a log.
I don't have time right now, but I can investigate later.