From: Alex Rousskov Date: Sun, 31 Oct 2021 01:37:23 +0000 (+0000) Subject: Fix HappyConnOpener::checkForNewConnection Must(prime) violation (#923) X-Git-Tag: SQUID_6_0_1~275 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=fbc253f2323689884f585837008afe0e6f7023bf;p=thirdparty%2Fsquid.git Fix HappyConnOpener::checkForNewConnection Must(prime) violation (#923) This change addresses a known problem that triggered unwanted C++ exceptions every time Squid selected a to-server persistent connection as the primary Happy Eyeballs destination/answer. The bug existed since HappyConnOpener inception (commit 5562295). It did not seem to affect the connection requestor directly because the HappyConnOpener job sends the selected pconn to the requestor _before_ throwing. Also adjusted Happy Eyeballs state documentation to reflect a successful termination state. Before and after this change, we may enter that state in the middle of checkForNewConnection(). The less we think about done() as an _exceptional_ "only on error" or "only at the end of processing" state, the fewer similar bugs we will create. The code also improved after we abandoned the idea of documenting all primary state changes in checkForNewConnection(). There are too many nuances/changes to document everything anyway, and moving primary track handling into a dedicated function significantly improves readability. --- diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc index 0436e35b6f..9c5c097aea 100644 --- a/src/HappyConnOpener.cc +++ b/src/HappyConnOpener.cc @@ -446,6 +446,7 @@ HappyConnOpener::status() const os << "spare:" << spare; if (n_tries) os << " tries:" << n_tries; + os << " dst:" << *destinations; os << ' ' << id << ']'; buf = os.buf(); @@ -690,14 +691,13 @@ HappyConnOpener::cancelSpareWait(const char *reason) /** Called when an external event changes initiator interest, destinations, * prime, spare, or spareWaiting. Leaves HappyConnOpener in one of these - * (mutually exclusive beyond the exceptional state #0) "stable" states: + * mutually exclusive "stable" states: * - * 0. Exceptional termination: done() - * 1. Processing a single peer: currentPeer + * 1. Processing a single peer: currentPeer && !done() * 1.1. Connecting: prime || spare * 1.2. Waiting for spare gap and/or paths: !prime && !spare - * 2. Waiting for a new peer: destinations->empty() && !destinations->destinationsFinalized && !currentPeer - * 3. Finished: destinations->empty() && destinations->destinationsFinalized && !currentPeer + * 2. Waiting for a new peer: destinations->empty() && !destinations->destinationsFinalized && !currentPeer && !done() + * 3. Terminating: done() */ void HappyConnOpener::checkForNewConnection() @@ -730,43 +730,13 @@ HappyConnOpener::checkForNewConnection() spareWaiting.forNewPeer = true; } - // open a new prime and/or a new spare connection if needed - if (!destinations->empty()) { - if (!currentPeer) { - auto newPrime = destinations->extractFront(); - currentPeer = newPrime; - Must(currentPeer); - debugs(17, 7, "new peer " << *currentPeer); - primeStart = current_dtime; - startConnecting(prime, newPrime); - // TODO: if reuseOldConnection() in startConnecting() above succeeds, - // then we should not get here, and Must(prime) below will fail. - maybeGivePrimeItsChance(); - Must(prime); // entering state #1.1 - } else { - if (!prime) - maybeOpenAnotherPrimeConnection(); // may make destinations empty() - } - - if (!spare && !spareWaiting) - maybeOpenSpareConnection(); // may make destinations empty() - - Must(currentPeer); - } - - if (currentPeer) { - debugs(17, 7, "working on " << *currentPeer); - return; // remaining in state #1.1 or #1.2 - } + if (!prime) + maybeOpenPrimeConnection(); - if (!destinations->destinationsFinalized) { - debugs(17, 7, "waiting for more peers"); - return; // remaining in state #2 - } + if (!spare && !done()) + maybeOpenSpareConnection(); - debugs(17, 7, "done; no more peers"); - Must(doneAll()); - // entering state #3 + // any state is possible at this point } void @@ -797,9 +767,30 @@ HappyConnOpener::noteSpareAllowance() /// starts a prime connection attempt if possible or does nothing otherwise void -HappyConnOpener::maybeOpenAnotherPrimeConnection() +HappyConnOpener::maybeOpenPrimeConnection() { - Must(currentPeer); + Must(!prime); + + if (destinations->empty()) + return; + + if (!currentPeer) { + auto newPrime = destinations->extractFront(); + currentPeer = newPrime; + Must(currentPeer); + debugs(17, 7, "new peer " << *currentPeer); + primeStart = current_dtime; + startConnecting(prime, newPrime); + if (done()) // probably reused a pconn + return; + + Must(prime); + maybeGivePrimeItsChance(); + return; + } + + // currentPeer implies there is a spare attempt; meanwhile, the previous + // primary attempt has failed; do another attempt on the primary track if (auto dest = destinations->extractPrime(*currentPeer)) startConnecting(prime, dest); // else wait for more prime paths or their exhaustion @@ -843,11 +834,16 @@ HappyConnOpener::maybeOpenSpareConnection() { Must(currentPeer); Must(!spare); - Must(!spareWaiting); Must(!gotSpareAllowance); + if (spareWaiting) + return; // too early + if (ranOutOfTimeOrAttempts()) - return; // will quit or continue working on prime + return; // too late + + if (destinations->empty()) + return; // jobGotInstantAllowance() call conditions below rely on the readyNow() check here if (!ignoreSpareRestrictions && // we have to honor spare restrictions diff --git a/src/HappyConnOpener.h b/src/HappyConnOpener.h index ff20232513..b7e69630a0 100644 --- a/src/HappyConnOpener.h +++ b/src/HappyConnOpener.h @@ -185,12 +185,12 @@ private: virtual void swanSong() override; virtual const char *status() const override; - void maybeOpenAnotherPrimeConnection(); + void maybeOpenPrimeConnection(); + void maybeOpenSpareConnection(); void maybeGivePrimeItsChance(); void stopGivingPrimeItsChance(); void stopWaitingForSpareAllowance(); - void maybeOpenSpareConnection(); void startConnecting(Attempt &, PeerConnectionPointer &); void openFreshConnection(Attempt &, PeerConnectionPointer &);