Minor test-infrastructure papercut, noticed while fixing #62 in #63.
Problem
PumpUntilBoth (Tests/Integration/SessionConfigLiveTests.cpp:80) deallocates and nulls both output packets when it times out:
if (*firstOut)
first->DeallocatePacket(*firstOut);
if (*secondOut)
second->DeallocatePacket(*secondOut);
*firstOut = 0;
*secondOut = 0;
return false;
So a caller gets a bare false and cannot tell whether one side arrived and the other did not, or neither did. Every call site collapses to a single message:
ASSERT_TRUE(PumpUntilBoth(client, ID_CONNECTION_REQUEST_ACCEPTED, &accepted, server, ID_NEW_INCOMING_CONNECTION, &incoming, kConnectTimeoutMs))
<< "the connection was not reported on both sides";
There are 9 call sites, 2 of them added by #63.
Why it is worth fixing
#62 was diagnosable from the failure output precisely because the old sequential code named the side: line 280 ... server never reported a connection pointed straight at which half of the handshake was missing. Converting to PumpUntilBoth was the right fix for the race, but it traded that signal away — a future timeout in any of these 9 places now starts from "something didn't arrive".
Because firstOut/secondOut are already nulled, the information exists at the moment of failure and is simply discarded.
Suggested fix
Have the helper say which side it was still waiting on, e.g. an optional out-param or a returned enum (kBoth / kFirstMissing / kSecondMissing / kNeither), and fold it into the assertion message. A cheap alternative that needs no signature change: fprintf(stderr, ...) naming the missing id(s) just before return false.
Both sides' ids are already in hand, so this is a few lines plus a sweep of the call sites. No behavioural change to any test.
Minor test-infrastructure papercut, noticed while fixing #62 in #63.
Problem
PumpUntilBoth(Tests/Integration/SessionConfigLiveTests.cpp:80) deallocates and nulls both output packets when it times out:So a caller gets a bare
falseand cannot tell whether one side arrived and the other did not, or neither did. Every call site collapses to a single message:There are 9 call sites, 2 of them added by #63.
Why it is worth fixing
#62 was diagnosable from the failure output precisely because the old sequential code named the side:
line 280 ... server never reported a connectionpointed straight at which half of the handshake was missing. Converting toPumpUntilBothwas the right fix for the race, but it traded that signal away — a future timeout in any of these 9 places now starts from "something didn't arrive".Because
firstOut/secondOutare already nulled, the information exists at the moment of failure and is simply discarded.Suggested fix
Have the helper say which side it was still waiting on, e.g. an optional out-param or a returned enum (
kBoth/kFirstMissing/kSecondMissing/kNeither), and fold it into the assertion message. A cheap alternative that needs no signature change:fprintf(stderr, ...)naming the missing id(s) just beforereturn false.Both sides' ids are already in hand, so this is a few lines plus a sweep of the call sites. No behavioural change to any test.