From 20df215e637491b366f80db325bc36da7531fbd3 Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 9 Sep 2024 10:46:04 -0600 Subject: [PATCH 01/29] add generation field to process --- .../Core/ApplicationPool/Group/InternalUtils.cpp | 4 ++-- src/agent/Core/ApplicationPool/Process.h | 11 ++++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/InternalUtils.cpp b/src/agent/Core/ApplicationPool/Group/InternalUtils.cpp index 034e43ce31..d8250332ad 100644 --- a/src/agent/Core/ApplicationPool/Group/InternalUtils.cpp +++ b/src/agent/Core/ApplicationPool/Group/InternalUtils.cpp @@ -178,7 +178,7 @@ Group::createNullProcessObject() { LockGuard l(context->memoryManagementSyncher); Process *process = context->processObjectPool.malloc(); Guard guard(context, process); - process = new (process) Process(&info, args); + process = new (process) Process(&info, info.group->restartsInitiated, args); process->shutdownNotRequired(); guard.clear(); return ProcessPtr(process, false); @@ -215,7 +215,7 @@ Group::createProcessObject(const SpawningKit::Spawner &spawner, LockGuard l(context->memoryManagementSyncher); Process *process = context->processObjectPool.malloc(); Guard guard(context, process); - process = new (process) Process(&info, spawnResult, args); + process = new (process) Process(&info, info.group->restartsInitiated, spawnResult, args); guard.clear(); return ProcessPtr(process, false); } diff --git a/src/agent/Core/ApplicationPool/Process.h b/src/agent/Core/ApplicationPool/Process.h index cd91f2e0e9..c5ad2700ae 100644 --- a/src/agent/Core/ApplicationPool/Process.h +++ b/src/agent/Core/ApplicationPool/Process.h @@ -384,6 +384,10 @@ class Process { /** Last time when a session was opened for this Process. */ unsigned long long lastUsed; + /** Which gereration of app processes this one belongs to, + inherited from the app group, incremented when a restart + is initiated*/ + const unsigned int generation; /** Number of sessions currently open. * @invariant session >= 0 */ @@ -446,8 +450,7 @@ class Process { /** Collected by Pool::collectAnalytics(). */ ProcessMetrics metrics; - - Process(const BasicGroupInfo *groupInfo, const Json::Value &args) + Process(const BasicGroupInfo *groupInfo, const unsigned int gen, const Json::Value &args) : info(this, groupInfo, args), socketsAcceptingHttpRequestsCount(0), spawnerCreationTime(getJsonUint64Field(args, "spawner_creation_time")), @@ -458,6 +461,7 @@ class Process { refcount(1), index(-1), lastUsed(spawnEndTime), + generation(gen), sessions(0), processed(0), lifeStatus(ALIVE), @@ -471,7 +475,7 @@ class Process { indexSocketsAcceptingHttpRequests(); } - Process(const BasicGroupInfo *groupInfo, const SpawningKit::Result &skResult, + Process(const BasicGroupInfo *groupInfo, const unsigned int gen, const SpawningKit::Result &skResult, const Json::Value &args) : info(this, groupInfo, skResult), socketsAcceptingHttpRequestsCount(0), @@ -483,6 +487,7 @@ class Process { refcount(1), index(-1), lastUsed(spawnEndTime), + generation(gen), sessions(0), processed(0), lifeStatus(ALIVE), From 744c2b289330fba25332cdfc2c832248dadf3122 Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 9 Sep 2024 11:18:28 -0600 Subject: [PATCH 02/29] use process generation to inform routing --- .../Group/ProcessListManagement.cpp | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index 94438ff766..88584b1b75 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -61,14 +61,20 @@ Process * Group::findProcessWithStickySessionIdOrLowestBusyness(unsigned int id) const { int leastBusyProcessIndex = -1; int lowestBusyness = 0; - unsigned int i, size = enabledProcessBusynessLevels.size(); + unsigned int i, size = enabledProcessBusynessLevels.size(), highest_gen = 0; const int *enabledProcessBusynessLevels = &this->enabledProcessBusynessLevels[0]; for (i = 0; i < size; i++) { Process *process = enabledProcesses[i].get(); + highest_gen = max(highest_gen, process->generation); if (process->getStickySessionId() == id) { return process; - } else if (leastBusyProcessIndex == -1 || enabledProcessBusynessLevels[i] < lowestBusyness) { + } else if (leastBusyProcessIndex == -1 || + enabledProcessBusynessLevels[i] < lowestBusyness || + (enabledProcessBusynessLevels[i] == lowestBusyness && process->generation > highest_gen)) { + if (process->generation > highest_gen) { + highest_gen = process->generation; + } leastBusyProcessIndex = i; lowestBusyness = enabledProcessBusynessLevels[i]; } @@ -88,13 +94,19 @@ Group::findProcessWithLowestBusyness(const ProcessList &processes) const { } int lowestBusyness = -1; + unsigned int highest_gen = 0; Process *leastBusyProcess = NULL; ProcessList::const_iterator it; ProcessList::const_iterator end = processes.end(); for (it = processes.begin(); it != end; it++) { Process *process = (*it).get(); int busyness = process->busyness(); - if (lowestBusyness == -1 || lowestBusyness > busyness) { + if (lowestBusyness == -1 || + lowestBusyness > busyness || + (busyness == lowestBusyness && process->generation > highest_gen)) { + if (process->generation > highest_gen) { + highest_gen = process->generation; + } lowestBusyness = busyness; leastBusyProcess = process; } @@ -113,11 +125,13 @@ Group::findEnabledProcessWithLowestBusyness() const { int leastBusyProcessIndex = -1; int lowestBusyness = 0; - unsigned int i, size = enabledProcessBusynessLevels.size(); + unsigned int i, size = enabledProcessBusynessLevels.size(), highest_gen = 0; const int *enabledProcessBusynessLevels = &this->enabledProcessBusynessLevels[0]; for (i = 0; i < size; i++) { - if (leastBusyProcessIndex == -1 || enabledProcessBusynessLevels[i] < lowestBusyness) { + if (leastBusyProcessIndex == -1 || + enabledProcessBusynessLevels[i] < lowestBusyness || + (enabledProcessBusynessLevels[i] == lowestBusyness && enabledProcesses[i]->generation > highest_gen)) { leastBusyProcessIndex = i; lowestBusyness = enabledProcessBusynessLevels[i]; } From b5dd97a9a981bd3395c018676e9c3fb3a0f6573e Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 9 Sep 2024 14:40:30 -0600 Subject: [PATCH 03/29] missed changes --- .../Core/ApplicationPool/Group/ProcessListManagement.cpp | 3 +++ test/cxx/Core/ApplicationPool/ProcessTest.cpp | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index 88584b1b75..c3a1aa5d46 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -132,6 +132,9 @@ Group::findEnabledProcessWithLowestBusyness() const { if (leastBusyProcessIndex == -1 || enabledProcessBusynessLevels[i] < lowestBusyness || (enabledProcessBusynessLevels[i] == lowestBusyness && enabledProcesses[i]->generation > highest_gen)) { + if (enabledProcesses[i]->generation > highest_gen) { + highest_gen = enabledProcesses[i]->generation; + } leastBusyProcessIndex = i; lowestBusyness = enabledProcessBusynessLevels[i]; } diff --git a/test/cxx/Core/ApplicationPool/ProcessTest.cpp b/test/cxx/Core/ApplicationPool/ProcessTest.cpp index 6e6518677b..b9d6c0727b 100644 --- a/test/cxx/Core/ApplicationPool/ProcessTest.cpp +++ b/test/cxx/Core/ApplicationPool/ProcessTest.cpp @@ -118,8 +118,10 @@ namespace tut { args["spawner_creation_time"] = 0; - ProcessPtr process(context.processObjectPool.construct( - &groupInfo, result, args), false); + Process *p = context.processObjectPool.malloc(); + p = new (p) Process(&groupInfo, 0, result, args); + ProcessPtr process(p, false); + process->shutdownNotRequired(); return process; } From df6f2ac3bd0abb25007b8504cad59f521cc80a69 Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Sun, 22 Sep 2024 13:58:35 -0600 Subject: [PATCH 04/29] address feedback --- .../Group/ProcessListManagement.cpp | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index c3a1aa5d46..72124c7530 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -61,20 +61,17 @@ Process * Group::findProcessWithStickySessionIdOrLowestBusyness(unsigned int id) const { int leastBusyProcessIndex = -1; int lowestBusyness = 0; - unsigned int i, size = enabledProcessBusynessLevels.size(), highest_gen = 0; + unsigned int i, size = enabledProcessBusynessLevels.size(), highestGeneration = 0; const int *enabledProcessBusynessLevels = &this->enabledProcessBusynessLevels[0]; for (i = 0; i < size; i++) { Process *process = enabledProcesses[i].get(); - highest_gen = max(highest_gen, process->generation); if (process->getStickySessionId() == id) { return process; - } else if (leastBusyProcessIndex == -1 || - enabledProcessBusynessLevels[i] < lowestBusyness || - (enabledProcessBusynessLevels[i] == lowestBusyness && process->generation > highest_gen)) { - if (process->generation > highest_gen) { - highest_gen = process->generation; - } + } else if (leastBusyProcessIndex == -1 || + enabledProcessBusynessLevels[i] < lowestBusyness || + (enabledProcessBusynessLevels[i] == lowestBusyness && process->generation > highestGeneration)) { + highestGeneration = std::max(highestGeneration, process->generation); leastBusyProcessIndex = i; lowestBusyness = enabledProcessBusynessLevels[i]; } @@ -94,7 +91,7 @@ Group::findProcessWithLowestBusyness(const ProcessList &processes) const { } int lowestBusyness = -1; - unsigned int highest_gen = 0; + unsigned int highestGeneration = 0; Process *leastBusyProcess = NULL; ProcessList::const_iterator it; ProcessList::const_iterator end = processes.end(); @@ -103,10 +100,8 @@ Group::findProcessWithLowestBusyness(const ProcessList &processes) const { int busyness = process->busyness(); if (lowestBusyness == -1 || lowestBusyness > busyness || - (busyness == lowestBusyness && process->generation > highest_gen)) { - if (process->generation > highest_gen) { - highest_gen = process->generation; - } + (busyness == lowestBusyness && process->generation > highestGeneration)) { + highestGeneration = std::max(highestGeneration, process->generation); lowestBusyness = busyness; leastBusyProcess = process; } @@ -125,16 +120,14 @@ Group::findEnabledProcessWithLowestBusyness() const { int leastBusyProcessIndex = -1; int lowestBusyness = 0; - unsigned int i, size = enabledProcessBusynessLevels.size(), highest_gen = 0; + unsigned int i, size = enabledProcessBusynessLevels.size(), highestGeneration = 0; const int *enabledProcessBusynessLevels = &this->enabledProcessBusynessLevels[0]; for (i = 0; i < size; i++) { if (leastBusyProcessIndex == -1 || enabledProcessBusynessLevels[i] < lowestBusyness || - (enabledProcessBusynessLevels[i] == lowestBusyness && enabledProcesses[i]->generation > highest_gen)) { - if (enabledProcesses[i]->generation > highest_gen) { - highest_gen = enabledProcesses[i]->generation; - } + (enabledProcessBusynessLevels[i] == lowestBusyness && enabledProcesses[i]->generation > highestGeneration)) { + highestGeneration = std::max(highestGeneration, enabledProcesses[i]->generation); leastBusyProcessIndex = i; lowestBusyness = enabledProcessBusynessLevels[i]; } From 38187b22dd1cbd6f1fb1e34a4933e2c6281d2f59 Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Sun, 22 Sep 2024 14:40:02 -0600 Subject: [PATCH 05/29] add a test to process test suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit not that there’s a point in this test really --- test/cxx/Core/ApplicationPool/ProcessTest.cpp | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/test/cxx/Core/ApplicationPool/ProcessTest.cpp b/test/cxx/Core/ApplicationPool/ProcessTest.cpp index b9d6c0727b..e90f2fb1a0 100644 --- a/test/cxx/Core/ApplicationPool/ProcessTest.cpp +++ b/test/cxx/Core/ApplicationPool/ProcessTest.cpp @@ -14,6 +14,7 @@ namespace tut { SpawningKit::Context skContext; Context context; BasicGroupInfo groupInfo; + unsigned int generation; vector sockets; Pipe stdinFd, stdoutAndErrFd; FileDescriptor server1, server2, server3; @@ -34,6 +35,8 @@ namespace tut { groupInfo.group = NULL; groupInfo.name = "test"; + generation = 0; + struct sockaddr_in addr; socklen_t len = sizeof(addr); SpawningKit::Result::Socket socket; @@ -119,7 +122,7 @@ namespace tut { args["spawner_creation_time"] = 0; Process *p = context.processObjectPool.malloc(); - p = new (p) Process(&groupInfo, 0, result, args); + p = new (p) Process(&groupInfo, generation, result, args); ProcessPtr process(p, false); process->shutdownNotRequired(); @@ -240,4 +243,15 @@ namespace tut { && contents.find("stdout and err 4\n") != string::npos; ); } + + TEST_METHOD(6) { + set_test_name("Test generation is inherited from pool at construction time"); + // this test is pointless b/c the process is made at line 123 of this file, not in the regular codebase + ProcessPtr process1 = createProcess(); + ensure_equals(process1->generation, generation); + generation++; + ProcessPtr process2 = createProcess(); + ensure_equals(process2->generation, generation); + ensure(process1->generation != process2->generation); + } } From a376e8eb6317565626ed60ce385c16507ab1fb82 Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 23 Sep 2024 14:58:53 -0600 Subject: [PATCH 06/29] add another test --- test/cxx/Core/ApplicationPool/PoolTest.cpp | 32 +++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index c2bc70a65d..a4be14c735 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -5,7 +5,6 @@ #include #include #include -#include #include #include #include @@ -632,6 +631,37 @@ namespace tut { ensure_equals(pool->getGroupCount(), 0u); } + TEST_METHOD(15) { + // Test that the process generation increments when the group restarts + Options options = createOptions(); + + // Spawn a process and opens a session with it. + pool->setMax(1); + pool->asyncGet(options, callback); + EVENTUALLY(5, + result = number == 1; + ); + + // Close the session so that the process is now idle. + ProcessPtr process = currentSession->getProcess()->shared_from_this(); + currentSession.reset(); + unsigned int gen1 = process->generation; + + ensure(pool->restartGroupByName(options.appRoot)); + EVENTUALLY(5, + result = pool->getProcessCount() == 1; + ); + pool->asyncGet(options, callback); + EVENTUALLY(5, + result = number == 2; + ); + + process = currentSession->getProcess()->shared_from_this(); + currentSession.reset(); + unsigned int gen2 = process->generation; + ensure_equals(gen1+1,gen2); + } + TEST_METHOD(17) { // Test that restartGroupByName() spawns more processes to ensure // that minProcesses and other constraints are met. From 824166d14a8fa8fee71bd9c2ca478236db5c9801 Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 23 Sep 2024 15:34:09 -0600 Subject: [PATCH 07/29] test stub for proper testing of routing algo --- test/cxx/Core/ApplicationPool/PoolTest.cpp | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index a4be14c735..3bb0e4ce44 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -662,6 +662,35 @@ namespace tut { ensure_equals(gen1+1,gen2); } + TEST_METHOD(16) { + // Test that the correct process from the pool is routed + Options options = createOptions(); + ensureMinProcesses(2); + + // async restart the group + ensure(pool->restartGroupByName(options.appRoot)); + ensureMinProcesses(1); + + /* + Imagine we have these processes (ordered from oldest to newest): + + #. PID 1 (generation A, busyness 5) + #. PID 2 (generation A, busyness 3) + #. PID 3 (generation B, busyness 1) + + The algorithm should select PID 3 + */ + + /* + Imagine we have these processes (ordered from oldest to newest): + + #. PID 1 (generation A, busyness 1) + #. PID 2 (generation B, busyness 5) + + The algorithm should select PID 1 + */ + } + TEST_METHOD(17) { // Test that restartGroupByName() spawns more processes to ensure // that minProcesses and other constraints are met. From 60d254b57202cfb8671f7cf20d34d885c45f5035 Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Fri, 27 Sep 2024 13:41:12 -0600 Subject: [PATCH 08/29] impl new algo --- src/agent/Core/ApplicationPool/Group.h | 6 +- .../Group/ProcessListManagement.cpp | 94 +++++++++---------- .../Group/SessionManagement.cpp | 8 +- src/agent/Core/ApplicationPool/Process.h | 21 ++--- 4 files changed, 64 insertions(+), 65 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group.h b/src/agent/Core/ApplicationPool/Group.h index fba45f8091..9f7362a1d4 100644 --- a/src/agent/Core/ApplicationPool/Group.h +++ b/src/agent/Core/ApplicationPool/Group.h @@ -223,9 +223,9 @@ class Group: public boost::enable_shared_from_this { /****** Process list management ******/ Process *findProcessWithStickySessionId(unsigned int id) const; - Process *findProcessWithStickySessionIdOrLowestBusyness(unsigned int id) const; - Process *findProcessWithLowestBusyness(const ProcessList &processes) const; - Process *findEnabledProcessWithLowestBusyness() const; + Process *findBestProcessPreferringStickySessionId(unsigned int id) const; + Process *findBestProcess(const ProcessList &processes) const; + Process *findBestEnabledProcess() const; void addProcessToList(const ProcessPtr &process, ProcessList &destination); void removeProcessFromList(const ProcessPtr &process, ProcessList &source); diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index 72124c7530..0c0544d52f 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -58,81 +58,81 @@ Group::findProcessWithStickySessionId(unsigned int id) const { } Process * -Group::findProcessWithStickySessionIdOrLowestBusyness(unsigned int id) const { - int leastBusyProcessIndex = -1; - int lowestBusyness = 0; - unsigned int i, size = enabledProcessBusynessLevels.size(), highestGeneration = 0; - const int *enabledProcessBusynessLevels = &this->enabledProcessBusynessLevels[0]; - - for (i = 0; i < size; i++) { - Process *process = enabledProcesses[i].get(); +Group::findBestProcessPreferringStickySessionId(unsigned int id) const { + Process *bestProcess = nullptr; + ProcessList::const_iterator it; + ProcessList::const_iterator end = enabledProcesses.end(); + for (it = enabledProcesses.begin(); it != end; it++) { + Process *process = (*it).get(); if (process->getStickySessionId() == id) { return process; - } else if (leastBusyProcessIndex == -1 || - enabledProcessBusynessLevels[i] < lowestBusyness || - (enabledProcessBusynessLevels[i] == lowestBusyness && process->generation > highestGeneration)) { - highestGeneration = std::max(highestGeneration, process->generation); - leastBusyProcessIndex = i; - lowestBusyness = enabledProcessBusynessLevels[i]; + } else if (bestProcess == nullptr || + process->generation > bestProcess->generation || + (process->generation == bestProcess->generation && process->spawnerCreationTime < bestProcess->spawnerCreationTime) || + (process->generation == bestProcess->generation && process->spawnerCreationTime == bestProcess->spawnerCreationTime && process->busyness() < bestProcess->busyness()) + ) { + bestProcess = process; } } - - if (leastBusyProcessIndex == -1) { - return NULL; - } else { - return enabledProcesses[leastBusyProcessIndex].get(); - } + return bestProcess; } Process * -Group::findProcessWithLowestBusyness(const ProcessList &processes) const { +Group::findBestProcess(const ProcessList &processes) const { if (processes.empty()) { - return NULL; + return nullptr; } - int lowestBusyness = -1; - unsigned int highestGeneration = 0; - Process *leastBusyProcess = NULL; + Process *bestProcess = nullptr; ProcessList::const_iterator it; ProcessList::const_iterator end = processes.end(); for (it = processes.begin(); it != end; it++) { Process *process = (*it).get(); - int busyness = process->busyness(); - if (lowestBusyness == -1 || - lowestBusyness > busyness || - (busyness == lowestBusyness && process->generation > highestGeneration)) { - highestGeneration = std::max(highestGeneration, process->generation); - lowestBusyness = busyness; - leastBusyProcess = process; + + if (bestProcess == nullptr || + process->generation > bestProcess->generation || + (process->generation == bestProcess->generation && process->spawnerCreationTime < bestProcess->spawnerCreationTime) || + (process->generation == bestProcess->generation && process->spawnerCreationTime == bestProcess->spawnerCreationTime && process->busyness() < bestProcess->busyness()) + ) { + bestProcess = process; } } - return leastBusyProcess; + return bestProcess; } /** - * Cache-optimized version of findProcessWithLowestBusyness() for the common case. + * Cache-optimized version of findBestProcess() for the common case. */ Process * -Group::findEnabledProcessWithLowestBusyness() const { +Group::findBestEnabledProcess() const { if (enabledProcesses.empty()) { - return NULL; + return nullptr; } - int leastBusyProcessIndex = -1; - int lowestBusyness = 0; - unsigned int i, size = enabledProcessBusynessLevels.size(), highestGeneration = 0; + Process* bestProcess = nullptr; + unsigned long long bestProcessStartTime = 0; + unsigned int bestProcessGen = 0; + int bestProcessBusyness = 0; + unsigned int size = enabledProcessBusynessLevels.size(); const int *enabledProcessBusynessLevels = &this->enabledProcessBusynessLevels[0]; - for (i = 0; i < size; i++) { - if (leastBusyProcessIndex == -1 || - enabledProcessBusynessLevels[i] < lowestBusyness || - (enabledProcessBusynessLevels[i] == lowestBusyness && enabledProcesses[i]->generation > highestGeneration)) { - highestGeneration = std::max(highestGeneration, enabledProcesses[i]->generation); - leastBusyProcessIndex = i; - lowestBusyness = enabledProcessBusynessLevels[i]; + for (unsigned int i = 0; i < size; i++) { + Process *process = enabledProcesses.at(i).get(); + unsigned int gen = process->generation; + unsigned long long startTime = process->spawnerCreationTime; + int busyness = enabledProcessBusynessLevels[i]; + if (bestProcess == nullptr || + gen > bestProcess->generation || + (gen == bestProcessGen && startTime < bestProcessStartTime) || + (gen == bestProcessGen && startTime == bestProcessStartTime && busyness < bestProcessBusyness) + ) { + bestProcess = process; + bestProcessGen = gen; + bestProcessBusyness = busyness; + bestProcessStartTime = startTime; } } - return enabledProcesses[leastBusyProcessIndex].get(); + return bestProcess; } /** diff --git a/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp b/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp index a9e9510077..be17dca6c3 100644 --- a/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp @@ -58,14 +58,14 @@ Group::RouteResult Group::route(const Options &options) const { if (OXT_LIKELY(enabledCount > 0)) { if (options.stickySessionId == 0) { - Process *process = findEnabledProcessWithLowestBusyness(); + Process *process = findBestEnabledProcess(); if (process->canBeRoutedTo()) { return RouteResult(process); } else { return RouteResult(NULL, true); } } else { - Process *process = findProcessWithStickySessionIdOrLowestBusyness( + Process *process = findBestProcessPreferringStickySessionId( options.stickySessionId); if (process != NULL) { if (process->canBeRoutedTo()) { @@ -78,7 +78,7 @@ Group::route(const Options &options) const { } } } else { - Process *process = findProcessWithLowestBusyness(disablingProcesses); + Process *process = findBestProcess(disablingProcesses); if (process->canBeRoutedTo()) { return RouteResult(process); } else { @@ -304,7 +304,7 @@ Group::get(const Options &newOptions, const GetCallback &callback, assert(m_spawning || restarting() || poolAtFullCapacity()); if (disablingCount > 0 && !restarting()) { - Process *process = findProcessWithLowestBusyness(disablingProcesses); + Process *process = findBestProcess(disablingProcesses); assert(process != NULL); if (!process->isTotallyBusy()) { return newSession(process, newOptions.currentTime); diff --git a/src/agent/Core/ApplicationPool/Process.h b/src/agent/Core/ApplicationPool/Process.h index c5ad2700ae..611a384fd5 100644 --- a/src/agent/Core/ApplicationPool/Process.h +++ b/src/agent/Core/ApplicationPool/Process.h @@ -28,7 +28,6 @@ #include #include -#include #include #include #include @@ -102,6 +101,12 @@ class Process { public: static const unsigned int MAX_SOCKETS_ACCEPTING_HTTP_REQUESTS = 3; + /** + * Time at which the Spawner that created this process was created. + * Microseconds resolution. + */ + unsigned long long spawnerCreationTime; + private: /************************************************************* * Read-only fields, set once during initialization and never @@ -141,12 +146,6 @@ class Process { */ StaticString codeRevision; - /** - * Time at which the Spawner that created this process was created. - * Microseconds resolution. - */ - unsigned long long spawnerCreationTime; - /** Time at which we started spawning this process. Microseconds resolution. */ unsigned long long spawnStartTime; @@ -451,9 +450,9 @@ class Process { ProcessMetrics metrics; Process(const BasicGroupInfo *groupInfo, const unsigned int gen, const Json::Value &args) - : info(this, groupInfo, args), + : spawnerCreationTime(getJsonUint64Field(args, "spawner_creation_time")), + info(this, groupInfo, args), socketsAcceptingHttpRequestsCount(0), - spawnerCreationTime(getJsonUint64Field(args, "spawner_creation_time")), spawnStartTime(getJsonUint64Field(args, "spawn_start_time")), spawnEndTime(SystemTime::getUsec()), type(args["type"] == "dummy" ? SpawningKit::Result::DUMMY : SpawningKit::Result::UNKNOWN), @@ -477,9 +476,9 @@ class Process { Process(const BasicGroupInfo *groupInfo, const unsigned int gen, const SpawningKit::Result &skResult, const Json::Value &args) - : info(this, groupInfo, skResult), + : spawnerCreationTime(getJsonUint64Field(args, "spawner_creation_time")), + info(this, groupInfo, skResult), socketsAcceptingHttpRequestsCount(0), - spawnerCreationTime(getJsonUint64Field(args, "spawner_creation_time")), spawnStartTime(skResult.spawnStartTime), spawnEndTime(skResult.spawnEndTime), type(skResult.type), From 6b739f36bda65b11e93800735139227fe7979e6c Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Sun, 29 Sep 2024 22:31:37 -0600 Subject: [PATCH 09/29] update changelog [ci skip] [ci:skip] --- CHANGELOG | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 3e1ee91260..f322903a7f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Release 6.0.24 (Not yet released) ------------- - * + * Update the order Passenger routes requests to app processes. Processes are now chosen based on being in the latest generation (Enterprise), then by newest process, then by oldest, then by +busyness. Closes GH-2551. Release 6.0.23 From fd217749bbdd2c77b694c4ccc0e8bc4aba4da6d0 Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 3 Oct 2024 15:51:20 +0200 Subject: [PATCH 10/29] Fix indentation --- .../Core/ApplicationPool/Group/ProcessListManagement.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index 0c0544d52f..5d5e304bfa 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -66,11 +66,11 @@ Group::findBestProcessPreferringStickySessionId(unsigned int id) const { Process *process = (*it).get(); if (process->getStickySessionId() == id) { return process; - } else if (bestProcess == nullptr || - process->generation > bestProcess->generation || + } else if (bestProcess == nullptr || + process->generation > bestProcess->generation || (process->generation == bestProcess->generation && process->spawnerCreationTime < bestProcess->spawnerCreationTime) || (process->generation == bestProcess->generation && process->spawnerCreationTime == bestProcess->spawnerCreationTime && process->busyness() < bestProcess->busyness()) - ) { + ) { bestProcess = process; } } @@ -93,7 +93,7 @@ Group::findBestProcess(const ProcessList &processes) const { process->generation > bestProcess->generation || (process->generation == bestProcess->generation && process->spawnerCreationTime < bestProcess->spawnerCreationTime) || (process->generation == bestProcess->generation && process->spawnerCreationTime == bestProcess->spawnerCreationTime && process->busyness() < bestProcess->busyness()) - ) { + ) { bestProcess = process; } } From c0d99a6ef4081e8a96cd4d78e7a8ca382c48f05f Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 3 Oct 2024 16:42:34 +0200 Subject: [PATCH 11/29] Remove the new test in ProcessTest.cpp because it's pointless --- test/cxx/Core/ApplicationPool/ProcessTest.cpp | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/test/cxx/Core/ApplicationPool/ProcessTest.cpp b/test/cxx/Core/ApplicationPool/ProcessTest.cpp index e90f2fb1a0..a3d508cd06 100644 --- a/test/cxx/Core/ApplicationPool/ProcessTest.cpp +++ b/test/cxx/Core/ApplicationPool/ProcessTest.cpp @@ -14,7 +14,6 @@ namespace tut { SpawningKit::Context skContext; Context context; BasicGroupInfo groupInfo; - unsigned int generation; vector sockets; Pipe stdinFd, stdoutAndErrFd; FileDescriptor server1, server2, server3; @@ -35,8 +34,6 @@ namespace tut { groupInfo.group = NULL; groupInfo.name = "test"; - generation = 0; - struct sockaddr_in addr; socklen_t len = sizeof(addr); SpawningKit::Result::Socket socket; @@ -122,9 +119,8 @@ namespace tut { args["spawner_creation_time"] = 0; Process *p = context.processObjectPool.malloc(); - p = new (p) Process(&groupInfo, generation, result, args); + p = new (p) Process(&groupInfo, 0, result, args); ProcessPtr process(p, false); - process->shutdownNotRequired(); return process; } @@ -243,15 +239,4 @@ namespace tut { && contents.find("stdout and err 4\n") != string::npos; ); } - - TEST_METHOD(6) { - set_test_name("Test generation is inherited from pool at construction time"); - // this test is pointless b/c the process is made at line 123 of this file, not in the regular codebase - ProcessPtr process1 = createProcess(); - ensure_equals(process1->generation, generation); - generation++; - ProcessPtr process2 = createProcess(); - ensure_equals(process2->generation, generation); - ensure(process1->generation != process2->generation); - } } From f8481e1dc254074de85a73e085d5047eac08083a Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 3 Oct 2024 16:44:45 +0200 Subject: [PATCH 12/29] Fix typo, set test name --- src/agent/Core/ApplicationPool/Process.h | 2 +- test/cxx/Core/ApplicationPool/PoolTest.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Process.h b/src/agent/Core/ApplicationPool/Process.h index 611a384fd5..0685e367a0 100644 --- a/src/agent/Core/ApplicationPool/Process.h +++ b/src/agent/Core/ApplicationPool/Process.h @@ -383,7 +383,7 @@ class Process { /** Last time when a session was opened for this Process. */ unsigned long long lastUsed; - /** Which gereration of app processes this one belongs to, + /** Which generation of app processes this one belongs to, inherited from the app group, incremented when a restart is initiated*/ const unsigned int generation; diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index 3bb0e4ce44..9a86b3ff86 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -632,7 +632,7 @@ namespace tut { } TEST_METHOD(15) { - // Test that the process generation increments when the group restarts + set_test_name("Process generation increments when the group restarts"); Options options = createOptions(); // Spawn a process and opens a session with it. From 6ab1b52d6d9c8c4885dda8c8932626c5ed4a3902 Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 3 Oct 2024 16:55:09 +0200 Subject: [PATCH 13/29] Use spawnStartTime instead of spawnerCreationTime --- .../Group/ProcessListManagement.cpp | 10 ++++----- src/agent/Core/ApplicationPool/Process.h | 22 ++++++++++--------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index 5d5e304bfa..8020020417 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -68,8 +68,8 @@ Group::findBestProcessPreferringStickySessionId(unsigned int id) const { return process; } else if (bestProcess == nullptr || process->generation > bestProcess->generation || - (process->generation == bestProcess->generation && process->spawnerCreationTime < bestProcess->spawnerCreationTime) || - (process->generation == bestProcess->generation && process->spawnerCreationTime == bestProcess->spawnerCreationTime && process->busyness() < bestProcess->busyness()) + (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) || + (process->generation == bestProcess->generation && process->spawnStartTime == bestProcess->spawnStartTime && process->busyness() < bestProcess->busyness()) ) { bestProcess = process; } @@ -91,8 +91,8 @@ Group::findBestProcess(const ProcessList &processes) const { if (bestProcess == nullptr || process->generation > bestProcess->generation || - (process->generation == bestProcess->generation && process->spawnerCreationTime < bestProcess->spawnerCreationTime) || - (process->generation == bestProcess->generation && process->spawnerCreationTime == bestProcess->spawnerCreationTime && process->busyness() < bestProcess->busyness()) + (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) || + (process->generation == bestProcess->generation && process->spawnStartTime == bestProcess->spawnStartTime && process->busyness() < bestProcess->busyness()) ) { bestProcess = process; } @@ -119,7 +119,7 @@ Group::findBestEnabledProcess() const { for (unsigned int i = 0; i < size; i++) { Process *process = enabledProcesses.at(i).get(); unsigned int gen = process->generation; - unsigned long long startTime = process->spawnerCreationTime; + unsigned long long startTime = process->spawnStartTime; int busyness = enabledProcessBusynessLevels[i]; if (bestProcess == nullptr || gen > bestProcess->generation || diff --git a/src/agent/Core/ApplicationPool/Process.h b/src/agent/Core/ApplicationPool/Process.h index 0685e367a0..7275402d6d 100644 --- a/src/agent/Core/ApplicationPool/Process.h +++ b/src/agent/Core/ApplicationPool/Process.h @@ -99,13 +99,9 @@ typedef boost::container::vector ProcessList; */ class Process { public: - static const unsigned int MAX_SOCKETS_ACCEPTING_HTTP_REQUESTS = 3; + friend class Group; - /** - * Time at which the Spawner that created this process was created. - * Microseconds resolution. - */ - unsigned long long spawnerCreationTime; + static const unsigned int MAX_SOCKETS_ACCEPTING_HTTP_REQUESTS = 3; private: /************************************************************* @@ -146,6 +142,12 @@ class Process { */ StaticString codeRevision; + /** + * Time at which the Spawner that created this process was created. + * Microseconds resolution. + */ + unsigned long long spawnerCreationTime; + /** Time at which we started spawning this process. Microseconds resolution. */ unsigned long long spawnStartTime; @@ -450,9 +452,9 @@ class Process { ProcessMetrics metrics; Process(const BasicGroupInfo *groupInfo, const unsigned int gen, const Json::Value &args) - : spawnerCreationTime(getJsonUint64Field(args, "spawner_creation_time")), - info(this, groupInfo, args), + : info(this, groupInfo, args), socketsAcceptingHttpRequestsCount(0), + spawnerCreationTime(getJsonUint64Field(args, "spawner_creation_time")), spawnStartTime(getJsonUint64Field(args, "spawn_start_time")), spawnEndTime(SystemTime::getUsec()), type(args["type"] == "dummy" ? SpawningKit::Result::DUMMY : SpawningKit::Result::UNKNOWN), @@ -476,9 +478,9 @@ class Process { Process(const BasicGroupInfo *groupInfo, const unsigned int gen, const SpawningKit::Result &skResult, const Json::Value &args) - : spawnerCreationTime(getJsonUint64Field(args, "spawner_creation_time")), - info(this, groupInfo, skResult), + : info(this, groupInfo, skResult), socketsAcceptingHttpRequestsCount(0), + spawnerCreationTime(getJsonUint64Field(args, "spawner_creation_time")), spawnStartTime(skResult.spawnStartTime), spawnEndTime(skResult.spawnEndTime), type(skResult.type), From cec763864668a8ca2d9987126120e92b176c8e86 Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 14 Oct 2024 13:13:38 -0600 Subject: [PATCH 14/29] remove unused headers --- src/agent/Core/ApplicationPool/Pool.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Pool.h b/src/agent/Core/ApplicationPool/Pool.h index 16ee56f928..e2c727db7f 100644 --- a/src/agent/Core/ApplicationPool/Pool.h +++ b/src/agent/Core/ApplicationPool/Pool.h @@ -28,10 +28,8 @@ #include #include -#include #include #include -#include #include #include #include From a45c981a935e1d70e83a413f20d3b76af2b774ae Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 14 Oct 2024 13:16:55 -0600 Subject: [PATCH 15/29] =?UTF-8?q?change=20test=20condition=20per=20hongli?= =?UTF-8?q?=E2=80=99s=20request?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/cxx/Core/ApplicationPool/PoolTest.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index 9a86b3ff86..bdda8bc6b6 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -644,12 +644,15 @@ namespace tut { // Close the session so that the process is now idle. ProcessPtr process = currentSession->getProcess()->shared_from_this(); + pid_t pid = process->getPid(); currentSession.reset(); unsigned int gen1 = process->generation; ensure(pool->restartGroupByName(options.appRoot)); EVENTUALLY(5, - result = pool->getProcessCount() == 1; + LockGuard l(pool->syncher); + vector processes = pool->getProcesses(false); + processes.size() > 0 && processes[0]->getPid() != pid; ); pool->asyncGet(options, callback); EVENTUALLY(5, From 8cff85c2bd2fba8b2e6afe0409f63c1e5a66caf8 Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 14 Oct 2024 13:25:05 -0600 Subject: [PATCH 16/29] =?UTF-8?q?don=E2=80=99t=20select=20a=20totally=20bu?= =?UTF-8?q?sy=20process?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ApplicationPool/Group/ProcessListManagement.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index 3758d198b0..6be6e7bee3 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -72,11 +72,11 @@ Group::findBestProcessPreferringStickySessionId(unsigned int id) const { Process *process = (*it).get(); if (process->getStickySessionId() == id) { return process; - } else if (bestProcess == nullptr || + } else if (!process->isTotallyBusy() && (bestProcess == nullptr || process->generation > bestProcess->generation || (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) || (process->generation == bestProcess->generation && process->spawnStartTime == bestProcess->spawnStartTime && process->busyness() < bestProcess->busyness()) - ) { + )) { bestProcess = process; } } @@ -95,11 +95,11 @@ Group::findBestProcess(const ProcessList &processes) const { for (it = processes.begin(); it != end; it++) { Process *process = (*it).get(); - if (bestProcess == nullptr || + if (!process->isTotallyBusy() && (bestProcess == nullptr || process->generation > bestProcess->generation || (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) || (process->generation == bestProcess->generation && process->spawnStartTime == bestProcess->spawnStartTime && process->busyness() < bestProcess->busyness()) - ) { + )) { bestProcess = process; } } @@ -127,11 +127,11 @@ Group::findBestEnabledProcess() const { unsigned int gen = process->generation; unsigned long long startTime = process->spawnStartTime; int busyness = enabledProcessBusynessLevels[i]; - if (bestProcess == nullptr || + if (!process->isTotallyBusy() && (bestProcess == nullptr || gen > bestProcess->generation || (gen == bestProcessGen && startTime < bestProcessStartTime) || (gen == bestProcessGen && startTime == bestProcessStartTime && busyness < bestProcessBusyness) - ) { + )) { bestProcess = process; bestProcessGen = gen; bestProcessBusyness = busyness; From b40c7d2d17292baa879a9a6be4213ad45f432a1c Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 14 Oct 2024 13:59:47 -0600 Subject: [PATCH 17/29] fallback to best possible process in case all are totally busy --- .../Group/ProcessListManagement.cpp | 49 ++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index 6be6e7bee3..c92c587205 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -66,12 +66,21 @@ Group::findProcessWithStickySessionId(unsigned int id) const { Process * Group::findBestProcessPreferringStickySessionId(unsigned int id) const { Process *bestProcess = nullptr; + Process *fallbackProcess = nullptr; ProcessList::const_iterator it; ProcessList::const_iterator end = enabledProcesses.end(); for (it = enabledProcesses.begin(); it != end; it++) { Process *process = (*it).get(); if (process->getStickySessionId() == id) { return process; + } else if (process->isTotallyBusy() && bestProcess == nullptr) { + if (fallbackProcess == nullptr || + process->generation > fallbackProcess->generation || + (process->generation == fallbackProcess->generation && process->spawnStartTime < fallbackProcess->spawnStartTime) || + (process->generation == fallbackProcess->generation && process->spawnStartTime == fallbackProcess->spawnStartTime && process->busyness() < fallbackProcess->busyness()) + ) { + fallbackProcess = process; + } } else if (!process->isTotallyBusy() && (bestProcess == nullptr || process->generation > bestProcess->generation || (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) || @@ -80,6 +89,9 @@ Group::findBestProcessPreferringStickySessionId(unsigned int id) const { bestProcess = process; } } + if (bestProcess == nullptr) { + return fallbackProcess; + } return bestProcess; } @@ -90,12 +102,21 @@ Group::findBestProcess(const ProcessList &processes) const { } Process *bestProcess = nullptr; + Process *fallbackProcess = nullptr; ProcessList::const_iterator it; ProcessList::const_iterator end = processes.end(); for (it = processes.begin(); it != end; it++) { Process *process = (*it).get(); - if (!process->isTotallyBusy() && (bestProcess == nullptr || + if (process->isTotallyBusy() && bestProcess == nullptr) { + if (fallbackProcess == nullptr || + process->generation > fallbackProcess->generation || + (process->generation == fallbackProcess->generation && process->spawnStartTime < fallbackProcess->spawnStartTime) || + (process->generation == fallbackProcess->generation && process->spawnStartTime == fallbackProcess->spawnStartTime && process->busyness() < fallbackProcess->busyness()) + ) { + fallbackProcess = process; + } + } else if (!process->isTotallyBusy() && (bestProcess == nullptr || process->generation > bestProcess->generation || (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) || (process->generation == bestProcess->generation && process->spawnStartTime == bestProcess->spawnStartTime && process->busyness() < bestProcess->busyness()) @@ -103,6 +124,9 @@ Group::findBestProcess(const ProcessList &processes) const { bestProcess = process; } } + if (bestProcess == nullptr) { + return fallbackProcess; + } return bestProcess; } @@ -119,6 +143,12 @@ Group::findBestEnabledProcess() const { unsigned long long bestProcessStartTime = 0; unsigned int bestProcessGen = 0; int bestProcessBusyness = 0; + + Process* fallbackProcess = nullptr; + unsigned long long fallbackProcessStartTime = 0; + unsigned int fallbackProcessGen = 0; + int fallbackProcessBusyness = 0; + unsigned int size = enabledProcessBusynessLevels.size(); const int *enabledProcessBusynessLevels = &this->enabledProcessBusynessLevels[0]; @@ -127,7 +157,19 @@ Group::findBestEnabledProcess() const { unsigned int gen = process->generation; unsigned long long startTime = process->spawnStartTime; int busyness = enabledProcessBusynessLevels[i]; - if (!process->isTotallyBusy() && (bestProcess == nullptr || + bool totallyBusy = process->isTotallyBusy(); + if (totallyBusy && bestProcess == nullptr) { + if (fallbackProcess == nullptr || + gen > fallbackProcess->generation || + (gen == fallbackProcessGen && startTime < fallbackProcessStartTime) || + (gen == fallbackProcessGen && startTime == fallbackProcessStartTime && busyness < fallbackProcessBusyness) + ) { + fallbackProcess = process; + fallbackProcessGen = gen; + fallbackProcessBusyness = busyness; + fallbackProcessStartTime = startTime; + } + } else if (!totallyBusy && (bestProcess == nullptr || gen > bestProcess->generation || (gen == bestProcessGen && startTime < bestProcessStartTime) || (gen == bestProcessGen && startTime == bestProcessStartTime && busyness < bestProcessBusyness) @@ -138,6 +180,9 @@ Group::findBestEnabledProcess() const { bestProcessStartTime = startTime; } } + if (bestProcess == nullptr) { + return fallbackProcess; + } return bestProcess; } From dec5f73dd6c31d32159a95ea5a39210f1d59d395 Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Thu, 17 Oct 2024 13:10:50 -0600 Subject: [PATCH 18/29] fix pool test for process generation incrementing --- test/cxx/Core/ApplicationPool/PoolTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index bdda8bc6b6..32c732f116 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -652,7 +652,7 @@ namespace tut { EVENTUALLY(5, LockGuard l(pool->syncher); vector processes = pool->getProcesses(false); - processes.size() > 0 && processes[0]->getPid() != pid; + result = (processes.size() > 0 && processes[0]->getPid() != pid); ); pool->asyncGet(options, callback); EVENTUALLY(5, From 6de82a7e59cd9da889631275534723429ffa487f Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Fri, 18 Oct 2024 10:59:31 -0600 Subject: [PATCH 19/29] try to make test 8 match new expectations of process selection --- test/cxx/Core/ApplicationPool/PoolTest.cpp | 27 ++++++++++++++-------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index 32c732f116..5b7022a1bc 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -434,6 +434,7 @@ namespace tut { EVENTUALLY(5, result = pool->getProcessCount() == 2; ); + ProcessPtr first_spawned_process = pool->getProcesses(false)[0]; // asyncGet() selects some process. pool->asyncGet(options, callback); @@ -442,31 +443,39 @@ namespace tut { ProcessPtr process1 = currentSession->getProcess()->shared_from_this(); currentSession.reset(); - // The first process now has 1 session, so next asyncGet() should - // select the other process. + // Next asyncGet() should select the process with the lowest spawnTime. pool->asyncGet(options, callback); ensure_equals("(2)", number, 2); SessionPtr session2 = currentSession; ProcessPtr process2 = currentSession->getProcess()->shared_from_this(); currentSession.reset(); - ensure("(3)", process1 != process2); + ensure_equals("(3)", process2, first_spawned_process); - // Both processes now have an equal number of sessions. Next asyncGet() - // can select either. + // Now that one process is totally busy, next asyncGet() should + // select the process that is not totally busy. pool->asyncGet(options, callback); ensure_equals("(4)", number, 3); SessionPtr session3 = currentSession; ProcessPtr process3 = currentSession->getProcess()->shared_from_this(); currentSession.reset(); + ensure("(5)", process3 != first_spawned_process); - // One process now has the lowest number of sessions. Next - // asyncGet() should select that one. + // Next asyncGet() should select the process that is not totally busy again. pool->asyncGet(options, callback); - ensure_equals("(5)", number, 4); + ensure_equals("(6)", number, 4); SessionPtr session4 = currentSession; ProcessPtr process4 = currentSession->getProcess()->shared_from_this(); currentSession.reset(); - ensure("(6)", process3 != process4); + ensure_equals("(7)", process3, process4); + + // Now that both processes are totally busy, next asyncGet() + // should select the process that has the lowest spawnTime. + pool->asyncGet(options, callback); + ensure_equals("(8)", number, 5); + SessionPtr session5 = currentSession; + ProcessPtr process5 = currentSession->getProcess()->shared_from_this(); + currentSession.reset(); + ensure_equals("(9)", process5, first_spawned_process); } TEST_METHOD(9) { From 3b0605ab2aa5807947ab68fe646092f7407076ff Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 21 Oct 2024 06:53:50 -0600 Subject: [PATCH 20/29] =?UTF-8?q?test=20doesn=E2=80=99t=20process=20reques?= =?UTF-8?q?ts=20on=20totallybusy=20processes=20obviously?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/cxx/Core/ApplicationPool/PoolTest.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index 5b7022a1bc..459c60379f 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -467,15 +467,6 @@ namespace tut { ProcessPtr process4 = currentSession->getProcess()->shared_from_this(); currentSession.reset(); ensure_equals("(7)", process3, process4); - - // Now that both processes are totally busy, next asyncGet() - // should select the process that has the lowest spawnTime. - pool->asyncGet(options, callback); - ensure_equals("(8)", number, 5); - SessionPtr session5 = currentSession; - ProcessPtr process5 = currentSession->getProcess()->shared_from_this(); - currentSession.reset(); - ensure_equals("(9)", process5, first_spawned_process); } TEST_METHOD(9) { From 7bceab1a3256e072bfa627f2e6c4fd540a4a7c45 Mon Sep 17 00:00:00 2001 From: Hongli Lai Date: Tue, 29 Oct 2024 17:39:47 +0100 Subject: [PATCH 21/29] Fix indenting and coding style [ci skip] --- .../Core/ApplicationPool/Group/ProcessListManagement.cpp | 4 ++-- test/cxx/Core/ApplicationPool/PoolTest.cpp | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index c92c587205..d04b3574d9 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -139,12 +139,12 @@ Group::findBestEnabledProcess() const { return nullptr; } - Process* bestProcess = nullptr; + Process *bestProcess = nullptr; unsigned long long bestProcessStartTime = 0; unsigned int bestProcessGen = 0; int bestProcessBusyness = 0; - Process* fallbackProcess = nullptr; + Process *fallbackProcess = nullptr; unsigned long long fallbackProcessStartTime = 0; unsigned int fallbackProcessGen = 0; int fallbackProcessBusyness = 0; diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index 459c60379f..e07386cad2 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -451,14 +451,14 @@ namespace tut { currentSession.reset(); ensure_equals("(3)", process2, first_spawned_process); - // Now that one process is totally busy, next asyncGet() should - // select the process that is not totally busy. + // Now that one process is totally busy, next asyncGet() should + // select the process that is not totally busy. pool->asyncGet(options, callback); ensure_equals("(4)", number, 3); SessionPtr session3 = currentSession; ProcessPtr process3 = currentSession->getProcess()->shared_from_this(); currentSession.reset(); - ensure("(5)", process3 != first_spawned_process); + ensure("(5)", process3 != first_spawned_process); // Next asyncGet() should select the process that is not totally busy again. pool->asyncGet(options, callback); From 03da6862b43bea4892202c659a7167d29a1c4fd7 Mon Sep 17 00:00:00 2001 From: Hongli Lai Date: Tue, 29 Oct 2024 17:41:20 +0100 Subject: [PATCH 22/29] Fix indenting [ci skip] --- test/cxx/Core/ApplicationPool/PoolTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index e07386cad2..4e180da905 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -434,7 +434,7 @@ namespace tut { EVENTUALLY(5, result = pool->getProcessCount() == 2; ); - ProcessPtr first_spawned_process = pool->getProcesses(false)[0]; + ProcessPtr first_spawned_process = pool->getProcesses(false)[0]; // asyncGet() selects some process. pool->asyncGet(options, callback); From c2b48f86c1a49e0935d8b976fbed4b9160628b4b Mon Sep 17 00:00:00 2001 From: Hongli Lai Date: Sun, 27 Oct 2024 12:49:37 +0100 Subject: [PATCH 23/29] Fix up test 8, improve code docs [ci skip] --- src/agent/Core/ApplicationPool/Group.h | 14 +++++++++-- .../Group/ProcessListManagement.cpp | 23 +++++++++++++++++-- .../Group/SessionManagement.cpp | 22 +++++++++++++++--- src/agent/Core/ApplicationPool/Process.h | 5 ++++ test/cxx/Core/ApplicationPool/PoolTest.cpp | 18 +++++++-------- test/tut/tut.h | 22 ++++++++++++++++++ 6 files changed, 88 insertions(+), 16 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group.h b/src/agent/Core/ApplicationPool/Group.h index 9f7362a1d4..5a33e55106 100644 --- a/src/agent/Core/ApplicationPool/Group.h +++ b/src/agent/Core/ApplicationPool/Group.h @@ -92,8 +92,18 @@ class Group: public boost::enable_shared_from_this { }; struct RouteResult { - Process *process; - bool finished; + /** The Process to route the request to, or nullptr if no process can be routed to. */ + Process * const process; + /** + * If `process` is nullptr, then `finished` indicates whether another `Group::route()` + * call on a different request *could* succeed, meaning that the caller should continue + * calling `Group::route()` if there are more queued requests that need to be processed. + * + * Usually `finished` is false because all processes are totally busy. But in some cases, + * for example when using sticky sessions, it could be true because other requests can + * potentially be routed to other processes. + */ + const bool finished; RouteResult(Process *p, bool _finished = false) : process(p), diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index d04b3574d9..8a5cc05034 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -63,6 +63,15 @@ Group::findProcessWithStickySessionId(unsigned int id) const { return NULL; } +/** + * Return the process with the given sticky session ID if it exists. + * If not, then find the "best" enabled process to route a request to, + * according to the same criteria documented for findBestProcess(). + * + * - If the process with the given sticky session ID exists, then always + * returns that process. Meaning that this process could be `!canBeRoutedTo()`. + * - If there is no process that be routed to, then returns nullptr. + */ Process * Group::findBestProcessPreferringStickySessionId(unsigned int id) const { Process *bestProcess = nullptr; @@ -70,7 +79,7 @@ Group::findBestProcessPreferringStickySessionId(unsigned int id) const { ProcessList::const_iterator it; ProcessList::const_iterator end = enabledProcesses.end(); for (it = enabledProcesses.begin(); it != end; it++) { - Process *process = (*it).get(); + Process *process = it->get(); if (process->getStickySessionId() == id) { return process; } else if (process->isTotallyBusy() && bestProcess == nullptr) { @@ -95,6 +104,15 @@ Group::findBestProcessPreferringStickySessionId(unsigned int id) const { return bestProcess; } +/** + * Given a ProcessList, find the "best" process to route a request to. + * At the moment, "best" is defined as the process with the highest generation, + * lowest start time, and lowest busyness, in that order of priority. + * + * If there is no process that be routed to, then returns nullptr. + * + * @post result != nullptr || result.canBeRoutedTo() + */ Process * Group::findBestProcess(const ProcessList &processes) const { if (processes.empty()) { @@ -131,7 +149,8 @@ Group::findBestProcess(const ProcessList &processes) const { } /** - * Cache-optimized version of findBestProcess() for the common case. + * Cache-optimized version of `findBestProcess()` for the common case. + * See `findBestProcess()` for the general contract. */ Process * Group::findBestEnabledProcess() const { diff --git a/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp b/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp index 09af45235d..e7c8cc0462 100644 --- a/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp @@ -65,7 +65,15 @@ Group::route(const Options &options) const { if (OXT_LIKELY(enabledCount > 0)) { if (options.stickySessionId == 0) { Process *process = findBestEnabledProcess(); - if (process->canBeRoutedTo()) { + if (process != nullptr) { + // TODO: remove this part when findBestEnabledProcess() has been + // modified to adhere to the new contract with postcondition + // `result != nullptr || result.canBeRoutedTo()` + if (!process->canBeRoutedTo()) { + return RouteResult(nullptr, false); + } + + assert(process->canBeRoutedTo()); return RouteResult(process); } else { return RouteResult(NULL, true); @@ -73,7 +81,7 @@ Group::route(const Options &options) const { } else { Process *process = findBestProcessPreferringStickySessionId( options.stickySessionId); - if (process != NULL) { + if (process != nullptr) { if (process->canBeRoutedTo()) { return RouteResult(process); } else { @@ -85,7 +93,15 @@ Group::route(const Options &options) const { } } else { Process *process = findBestProcess(disablingProcesses); - if (process->canBeRoutedTo()) { + if (process != nullptr) { + // TODO: remove this part when findBestProcess() has been + // modified to adhere to the new contract with postcondition + // `result != nullptr || result.canBeRoutedTo()` + if (!process->canBeRoutedTo()) { + return RouteResult(nullptr, false); + } + + assert(process->canBeRoutedTo()); return RouteResult(process); } else { return RouteResult(NULL, true); diff --git a/src/agent/Core/ApplicationPool/Process.h b/src/agent/Core/ApplicationPool/Process.h index 72dd13599b..ecae0a135a 100644 --- a/src/agent/Core/ApplicationPool/Process.h +++ b/src/agent/Core/ApplicationPool/Process.h @@ -53,6 +53,10 @@ #include #include +namespace tut { + template class test_object; +} + namespace Passenger { namespace ApplicationPool2 { @@ -100,6 +104,7 @@ typedef boost::container::vector ProcessList; class Process { public: friend class Group; + template friend class tut::test_object; static const unsigned int MAX_SOCKETS_ACCEPTING_HTTP_REQUESTS = 3; diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index 4e180da905..74fc3412b0 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -434,7 +434,6 @@ namespace tut { EVENTUALLY(5, result = pool->getProcessCount() == 2; ); - ProcessPtr first_spawned_process = pool->getProcesses(false)[0]; // asyncGet() selects some process. pool->asyncGet(options, callback); @@ -443,24 +442,25 @@ namespace tut { ProcessPtr process1 = currentSession->getProcess()->shared_from_this(); currentSession.reset(); - // Next asyncGet() should select the process with the lowest spawnTime. + // The first process now has 1 session. Next asyncGet() should + // select the same process because it's not totally busy. pool->asyncGet(options, callback); ensure_equals("(2)", number, 2); SessionPtr session2 = currentSession; ProcessPtr process2 = currentSession->getProcess()->shared_from_this(); currentSession.reset(); - ensure_equals("(3)", process2, first_spawned_process); + ensure("(3)", process1 == process2); // Now that one process is totally busy, next asyncGet() should - // select the process that is not totally busy. + // select the other process. pool->asyncGet(options, callback); ensure_equals("(4)", number, 3); SessionPtr session3 = currentSession; ProcessPtr process3 = currentSession->getProcess()->shared_from_this(); currentSession.reset(); - ensure("(5)", process3 != first_spawned_process); + ensure_not_equals("(5)", process3, process1); - // Next asyncGet() should select the process that is not totally busy again. + // Next asyncGet() should select the other process again. pool->asyncGet(options, callback); ensure_equals("(6)", number, 4); SessionPtr session4 = currentSession; @@ -650,9 +650,9 @@ namespace tut { ensure(pool->restartGroupByName(options.appRoot)); EVENTUALLY(5, - LockGuard l(pool->syncher); - vector processes = pool->getProcesses(false); - result = (processes.size() > 0 && processes[0]->getPid() != pid); + LockGuard l(pool->syncher); + vector processes = pool->getProcesses(false); + result = (processes.size() > 0 && processes[0]->getPid() != pid); ); pool->asyncGet(options, callback); EVENTUALLY(5, diff --git a/test/tut/tut.h b/test/tut/tut.h index 23ebcca85f..016117c065 100644 --- a/test/tut/tut.h +++ b/test/tut/tut.h @@ -815,6 +815,28 @@ void ensure_equals(const Q& actual, const T& expected) ensure_equals<>(0, actual, expected); } +/** + * Tests two objects for not being equal. + * Throws if equal. + * + * NB: T must have operator << defined somewhere, or + * client code will not compile at all! + */ +template +void ensure_not_equals(const char* msg, const Q& a, const T& b) +{ + if (a == b) + { + std::stringstream ss; + ss << (msg ? msg : "") + << (msg ? ":" : "") + << " expected not to be '" + << a + << "'"; + throw failure(ss.str().c_str()); + } +} + /** * Tests two objects for being at most in given distance one from another. * Borders are excluded. From c76fb2d5658535905d044f4521ed7fbecaea2257 Mon Sep 17 00:00:00 2001 From: Hongli Lai Date: Sun, 27 Oct 2024 15:01:37 +0100 Subject: [PATCH 24/29] Tests: fix detecting Python on systems with only "python3" command --- test/cxx/Core/ApplicationPool/PoolTest.cpp | 3 ++ test/cxx/TestSupport.cpp | 36 +++++++++++++++++++++- test/cxx/TestSupport.h | 24 +++++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index 74fc3412b0..2f65c722f1 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -23,6 +23,7 @@ namespace tut { Context context; PoolPtr pool; Pool::DebugSupportPtr debug; + string pythonCommand; Ticket ticket; GetCallback callback; SessionPtr currentSession; @@ -47,6 +48,7 @@ namespace tut { context.finalize(); pool = boost::make_shared(&context); pool->initialize(); + pythonCommand = findPythonCommand(); callback.func = _callback; callback.userData = this; @@ -113,6 +115,7 @@ namespace tut { options.spawnMethod = "dummy"; options.appRoot = "stub/rack"; options.appType = "ruby"; + options.python = pythonCommand.c_str(); options.appStartCommand = "ruby start.rb"; options.startupFile = "start.rb"; options.loadShellEnvvars = false; diff --git a/test/cxx/TestSupport.cpp b/test/cxx/TestSupport.cpp index 5955f17ea5..b624a32714 100644 --- a/test/cxx/TestSupport.cpp +++ b/test/cxx/TestSupport.cpp @@ -7,9 +7,11 @@ #include #include #include -#include +#include +#include #include #include +#include #include #include @@ -36,6 +38,38 @@ createInstanceDir(InstanceDirectoryPtr &instanceDir) { instanceDir = boost::make_shared(options); } +string +findCommandInPath(const string &basename) { + const char *path = getenv("PATH"); + if (path == nullptr) { + return string(); + } + + vector directories; + split(path, ':', directories); + + for (const string &dir: directories) { + string fullPath = dir + "/" + basename; + if (fileExists(fullPath)) { + return fullPath; + } + } + + return string(); +} + +string +findPythonCommand() { + string python = findCommandInPath("python"); + if (python.empty()) { + python = findCommandInPath("python3"); + } + if (python.empty()) { + python = findCommandInPath("python2"); + } + return python; +} + void writeUntilFull(int fd) { int flags, ret; diff --git a/test/cxx/TestSupport.h b/test/cxx/TestSupport.h index 872f2127d3..46267e4f3a 100644 --- a/test/cxx/TestSupport.h +++ b/test/cxx/TestSupport.h @@ -106,6 +106,30 @@ extern Json::Value testConfig; */ void createInstanceDir(InstanceDirectoryPtr &instanceDir); +/** + * Find the given command in PATH. Returns the empty string if not found. + * + * @throws FileSystemException + * @throws TimeRetrievalException + * @throws boost::thread_interrupted + */ +string findCommandInPath(const string &basename); + +/** + * Find an appropriate Python command for this system, regardless of whether it's Python 2 or 3. + * Returns something like "/usr/bin/python3" if possible, but on older + * systems without Python 3 it could return something like "/usr/bin/python". + * + * This function primarily exists because newer systems don't have a "python" command anymore, only "python3". + * + * Returns the empty string if not found. + * + * @throws FileSystemException + * @throws TimeRetrievalException + * @throws boost::thread_interrupted + */ +string findPythonCommand(); + /** * Writes zeroes into the given file descriptor its buffer is full (i.e. * the next write will block). From d613e48bfcc2ec903ea98198e4a1a76aa939cb1f Mon Sep 17 00:00:00 2001 From: Hongli Lai Date: Fri, 1 Nov 2024 17:18:35 +0100 Subject: [PATCH 25/29] Improve tests --- .../Group/SessionManagement.cpp | 1 + test/cxx/Core/ApplicationPool/PoolTest.cpp | 448 ++++++++++++------ test/tut/tut.h | 24 + 3 files changed, 322 insertions(+), 151 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp b/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp index e7c8cc0462..1c5ee53512 100644 --- a/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp @@ -30,6 +30,7 @@ #include #endif #include +#include /************************************************************************* * diff --git a/test/cxx/Core/ApplicationPool/PoolTest.cpp b/test/cxx/Core/ApplicationPool/PoolTest.cpp index 2f65c722f1..6a39853c46 100644 --- a/test/cxx/Core/ApplicationPool/PoolTest.cpp +++ b/test/cxx/Core/ApplicationPool/PoolTest.cpp @@ -207,11 +207,12 @@ namespace tut { // Ensure that n processes exist. Options ensureMinProcesses(unsigned int n) { + int prevNumber = number; Options options = createOptions(); options.minProcesses = n; pool->asyncGet(options, callback); EVENTUALLY(5, - result = number == 1; + result = number == prevNumber + 1; ); EVENTUALLY(5, result = pool->getProcessCount() == n; @@ -261,7 +262,7 @@ namespace tut { TEST_METHOD(3) { // If one matching process already exists and it's not at full - // capacity then asyncGet() will immediately use it. + // utilization, then asyncGet() will immediately use it. Options options = createOptions(); // Spawn a process and opens a session with it. @@ -283,13 +284,13 @@ namespace tut { } TEST_METHOD(4) { - // If one matching process already exists but it's at full capacity, + // If one matching process already exists but it's at full utilization, // and the limits prevent spawning of a new process, // then asyncGet() will put the get action on the group's wait - // queue. When the process is no longer at full capacity it will + // queue. When the process is no longer at full utilization it will // process the request. - // Spawn a process and verify that it's at full capacity. + // Spawn a process and verify that it's at full utilization. // Keep its session open. Options options = createOptions(); options.appGroupName = "test"; @@ -379,104 +380,278 @@ namespace tut { } TEST_METHOD(7) { - // If multiple matching processes exist, and one of them is idle, - // then asyncGet() will use that. + set_test_name("If multiple matching processes exist, none of them at full utilization," + " then asyncGet() routes to the process with the highest generation number"); + + // Spawn a process with generation 0. + ensureMinProcesses(1); + ProcessPtr process1 = pool->getProcesses()[0]; + ensure_equals("(1)", process1->generation, 0u); - // Spawn 3 processes and keep a session open with 1 of them. + // Spawn another process with generation 1. + { + LockGuard l(pool->syncher); + process1->getGroup()->restartsInitiated = 1; + } + ensureMinProcesses(2); + ProcessPtr process2 = pool->getProcesses()[1]; + ensure_equals("(2)", process2->generation, 1u); + + // asyncGet() should select the process with the highest generation. Options options = createOptions(); - options.minProcesses = 3; pool->asyncGet(options, callback); EVENTUALLY(5, - result = number == 1; - ); - EVENTUALLY(5, - result = pool->getProcessCount() == 3; + result = number == 3; ); - SessionPtr session1 = currentSession; - ProcessPtr process1 = currentSession->getProcess()->shared_from_this(); + ProcessPtr process3 = currentSession->getProcess()->shared_from_this(); + ensure_equals("(3)", process3, process2); currentSession.reset(); + } - // Now open another session. It should complete immediately - // and should not use the first process. - ScopedLock l(pool->syncher); - pool->asyncGet(options, callback, false); - ensure_equals("asyncGet() completed immediately", number, 2); - SessionPtr session2 = currentSession; - ProcessPtr process2 = currentSession->getProcess()->shared_from_this(); - l.unlock(); - currentSession.reset(); - ensure(process2 != process1); + TEST_METHOD(8) { + set_test_name("If multiple matching processes exist, none of them at full utilization," + " all of the same generation, then asyncGet() routes to the process with" + " the earliest start time"); - // Now open yet another session. It should also complete immediately - // and should not use the first or the second process. - l.lock(); - pool->asyncGet(options, callback, false); - ensure_equals("asyncGet() completed immediately", number, 3); - SessionPtr session3 = currentSession; + // Spawn two processes with the same generation number. + SystemTime::forceAll(1000000); + ensureMinProcesses(1); + SystemTime::forceAll(2000000); + ensureMinProcesses(2); + SystemTime::releaseAll(); + ProcessPtr process1 = pool->getProcesses()[0]; + ProcessPtr process2 = pool->getProcesses()[1]; + ensure_gt("(1)", process2->spawnStartTime, process1->spawnStartTime); + + // asyncGet() should select the oldest process. + Options options = createOptions(); + pool->asyncGet(options, callback); + EVENTUALLY(5, + result = number == 3; + ); ProcessPtr process3 = currentSession->getProcess()->shared_from_this(); - l.unlock(); + ensure_equals("(2)", process3, process1); currentSession.reset(); - ensure(process3 != process1); - ensure(process3 != process2); } - TEST_METHOD(8) { - // If multiple matching processes exist, then asyncGet() will use - // the one with the smallest utilization number. + TEST_METHOD(9) { + set_test_name("If multiple matching processes exist, none of them at full utilization," + " all of the same generation and start time, then asyncGet() routes to" + " the process with the lowest busyness"); - // Spawn 2 processes, each with a concurrency of 2. - skDebugSupport.dummyConcurrency = 2; + // Spawn three processes with the same generation number and start time, + // all with a concurrency of 3. + SystemTime::forceAll(1000000); + skDebugSupport.dummyConcurrency = 3; + ensureMinProcesses(3); + SystemTime::releaseAll(); + ProcessPtr process1 = pool->getProcesses()[0]; + ProcessPtr process2 = pool->getProcesses()[1]; + ProcessPtr process3 = pool->getProcesses()[2]; + ensure_equals("(1)", process2->spawnStartTime, process1->spawnStartTime); + ensure_equals("(2)", process3->spawnStartTime, process1->spawnStartTime); + + // Create the following situation: + // process1: sessions=2 + // process2: sessions=1 + // process3: sessions=2 + + // First open 9 sessions. Options options = createOptions(); - options.minProcesses = 2; - pool->setMax(2); - GroupPtr group = pool->findOrCreateGroup(options); + vector sessions; + int prevNumber = number; + for (unsigned int i = 0; i < 9; i++) { + pool->asyncGet(options, callback); + EVENTUALLY(5, + result = number == static_cast(prevNumber + i + 1); + ); + sessions.push_back(currentSession); + currentSession.reset(); + } + + // Check various sessions belong to the expected processes. + ensure_equals("(3)", sessions[0]->getProcess()->getPid(), process1->getPid()); + ensure_equals("(4)", sessions[1]->getProcess()->getPid(), process2->getPid()); + ensure_equals("(5)", sessions[2]->getProcess()->getPid(), process3->getPid()); + ensure_equals("(6)", sessions[4]->getProcess()->getPid(), process2->getPid()); + + // Close a bunch of sessions and we should arrive at the desired situation. + sessions[0].reset(); + sessions[1].reset(); + sessions[2].reset(); + sessions[4].reset(); { LockGuard l(pool->syncher); - group->spawn(); + ensure_equals("(7)", process1->sessions, 2); + ensure_equals("(8)", process2->sessions, 1); + ensure_equals("(9)", process3->sessions, 2); } - EVENTUALLY(5, - result = pool->getProcessCount() == 2; - ); - // asyncGet() selects some process. + // asyncGet() should select process 2 (least busyness). pool->asyncGet(options, callback); - ensure_equals("(1)", number, 1); - SessionPtr session1 = currentSession; - ProcessPtr process1 = currentSession->getProcess()->shared_from_this(); + prevNumber = number; + EVENTUALLY(5, + result = prevNumber + 1; + ); + ProcessPtr process = currentSession->getProcess()->shared_from_this(); + ensure_equals("(10)", process->getPid(), process2->getPid()); currentSession.reset(); + } - // The first process now has 1 session. Next asyncGet() should - // select the same process because it's not totally busy. - pool->asyncGet(options, callback); - ensure_equals("(2)", number, 2); - SessionPtr session2 = currentSession; - ProcessPtr process2 = currentSession->getProcess()->shared_from_this(); - currentSession.reset(); - ensure("(3)", process1 == process2); + TEST_METHOD(10) { + set_test_name("If multiple matching processes exist," + " with generation 3 processes at full utilization," + " generation 2 and generation 1 processes not at full utilization," + " then asyncGet() routes to generation 2"); + + skDebugSupport.dummyConcurrency = 2; - // Now that one process is totally busy, next asyncGet() should - // select the other process. + // Spawn a process with generation 0. + ensureMinProcesses(1); + ProcessPtr process1 = pool->getProcesses()[0]; + GroupPtr group = process1->getGroup()->shared_from_this(); + ensure_equals("(1)", process1->generation, 0u); + + // Spawn another process with generation 2. + { + LockGuard l(pool->syncher); + group->restartsInitiated = 2; + } + ensureMinProcesses(2); + ProcessPtr process2 = pool->getProcesses()[1]; + ensure_equals("(2)", process2->generation, 2u); + + // Spawn another process with generation 3. + { + LockGuard l(pool->syncher); + group->restartsInitiated = 3; + } + ensureMinProcesses(3); + ProcessPtr process3 = pool->getProcesses()[2]; + ensure_equals("(3)", process3->generation, 3u); + + // Create the following situation: + // process1: sessions=0 + // process2: sessions=1 + // process3: sessions=2 (full utilization) + + // First open 6 sessions. + Options options = createOptions(); + vector sessions; + int prevNumber = number; + for (unsigned int i = 0; i < 6; i++) { + pool->asyncGet(options, callback); + EVENTUALLY(5, + result = number == static_cast(prevNumber + i + 1); + ); + sessions.push_back(currentSession); + currentSession.reset(); + } + + // Check various sessions belong to the expected processes. + ensure_equals("(4)", sessions[0]->getProcess()->getPid(), process3->getPid()); + ensure_equals("(5)", sessions[1]->getProcess()->getPid(), process3->getPid()); + ensure_equals("(6)", sessions[2]->getProcess()->getPid(), process2->getPid()); + ensure_equals("(7)", sessions[3]->getProcess()->getPid(), process2->getPid()); + ensure_equals("(8)", sessions[4]->getProcess()->getPid(), process1->getPid()); + ensure_equals("(9)", sessions[5]->getProcess()->getPid(), process1->getPid()); + + // Close a bunch of sessions and we should arrive at the desired situation. + sessions[3].reset(); + sessions[4].reset(); + sessions[5].reset(); + { + LockGuard l(pool->syncher); + ensure_equals("(10)", process1->sessions, 0); + ensure_equals("(11)", process2->sessions, 1); + ensure_equals("(12)", process3->sessions, 2); + } + + // asyncGet() should select process2 even though process1 has fewer sessions open and is older. pool->asyncGet(options, callback); - ensure_equals("(4)", number, 3); - SessionPtr session3 = currentSession; - ProcessPtr process3 = currentSession->getProcess()->shared_from_this(); + prevNumber = number; + EVENTUALLY(5, + result = number + 1; + ); + ProcessPtr process4 = currentSession->getProcess()->shared_from_this(); + ensure_equals("(13)", process4->getPid(), process2->getPid()); currentSession.reset(); - ensure_not_equals("(5)", process3, process1); + } + + TEST_METHOD(11) { + set_test_name("If multiple matching processes exist, all of them of the same generation," + " with the lowest-start-time processes being at full utilization" + " and higher-start-time processes not at full utilization," + " then asyncGet() routes to a lowest-start-time process that's not at full utilization"); + + skDebugSupport.dummyConcurrency = 2; - // Next asyncGet() should select the other process again. + SystemTime::forceAll(1000000); + ensureMinProcesses(1); + SystemTime::forceAll(2000000); + ensureMinProcesses(2); + SystemTime::forceAll(3000000); + ensureMinProcesses(3); + SystemTime::releaseAll(); + ProcessPtr process1 = pool->getProcesses()[0]; + ProcessPtr process2 = pool->getProcesses()[1]; + ProcessPtr process3 = pool->getProcesses()[2]; + ensure_gt("(1)", process2->spawnStartTime, process1->spawnStartTime); + ensure_gt("(2)", process3->spawnStartTime, process2->spawnStartTime); + + // Create the following situation: + // process1: sessions=2 (full utilization) + // process2: sessions=1 + // process3: sessions=0 + + // First open 6 sessions. + Options options = createOptions(); + vector sessions; + int prevNumber = number; + for (unsigned int i = 0; i < 6; i++) { + pool->asyncGet(options, callback); + EVENTUALLY(5, + result = number == static_cast(prevNumber + i + 1); + ); + sessions.push_back(currentSession); + currentSession.reset(); + } + + // Check various sessions belong to the expected processes. + ensure_equals("(4)", sessions[0]->getProcess()->getPid(), process1->getPid()); + ensure_equals("(5)", sessions[1]->getProcess()->getPid(), process1->getPid()); + ensure_equals("(6)", sessions[2]->getProcess()->getPid(), process2->getPid()); + ensure_equals("(7)", sessions[3]->getProcess()->getPid(), process2->getPid()); + ensure_equals("(8)", sessions[4]->getProcess()->getPid(), process3->getPid()); + ensure_equals("(9)", sessions[5]->getProcess()->getPid(), process3->getPid()); + + // Close a bunch of sessions and we should arrive at the desired situation. + sessions[3].reset(); + sessions[4].reset(); + sessions[5].reset(); + { + LockGuard l(pool->syncher); + ensure_equals("(10)", process1->sessions, 2); + ensure_equals("(11)", process2->sessions, 1); + ensure_equals("(12)", process3->sessions, 0); + } + + // asyncGet() should select process2 (oldest possible process) even though process3 has fewer sessions open. pool->asyncGet(options, callback); - ensure_equals("(6)", number, 4); - SessionPtr session4 = currentSession; + prevNumber = number; + EVENTUALLY(5, + result = number + 1; + ); ProcessPtr process4 = currentSession->getProcess()->shared_from_this(); + ensure_equals("(13)", process4->getPid(), process2->getPid()); currentSession.reset(); - ensure_equals("(7)", process3, process4); } - TEST_METHOD(9) { - // If multiple matching processes exist, and all of them are at full capacity, + TEST_METHOD(13) { + // If multiple matching processes exist, and all of them are at full utilization, // and no more processes may be spawned, // then asyncGet() will put the action on the group's wait queue. - // The process that first becomes not at full capacity will process the action. + // The process that first becomes not at full utilization will process the action. // Spawn 2 processes and open 4 sessions. Options options = createOptions(); @@ -517,7 +692,7 @@ namespace tut { ensure(pool->atFullCapacity()); } - TEST_METHOD(10) { + TEST_METHOD(14) { // If multiple matching processes exist, and all of them are at full utilization, // and a new process may be spawned, // then asyncGet() will put the action on the group's wait queue and spawn the @@ -566,7 +741,7 @@ namespace tut { ensure_equals(pool->getProcessCount(), 2u); } - TEST_METHOD(11) { + TEST_METHOD(15) { // Here we test the case where the newly spawned process is earlier. // Spawn 2 processes and open 4 sessions. @@ -604,14 +779,14 @@ namespace tut { ensure_equals(group->getWaitlist.size(), 0u); } - TEST_METHOD(12) { + TEST_METHOD(16) { // Test shutting down. ensureMinProcesses(2); ensure(pool->detachGroupByName("stub/rack")); ensure_equals(pool->getGroupCount(), 0u); } - TEST_METHOD(13) { + TEST_METHOD(17) { // Test shutting down while Group is restarting. initPoolDebugging(); debug->messages->send("Proceed with spawn loop iteration 1"); @@ -623,7 +798,7 @@ namespace tut { ensure_equals(pool->getGroupCount(), 0u); } - TEST_METHOD(14) { + TEST_METHOD(18) { // Test shutting down while Group is spawning. initPoolDebugging(); Options options = createOptions(); @@ -634,7 +809,7 @@ namespace tut { ensure_equals(pool->getGroupCount(), 0u); } - TEST_METHOD(15) { + TEST_METHOD(19) { set_test_name("Process generation increments when the group restarts"); Options options = createOptions(); @@ -651,7 +826,7 @@ namespace tut { currentSession.reset(); unsigned int gen1 = process->generation; - ensure(pool->restartGroupByName(options.appRoot)); + ensure("(1)", pool->restartGroupByName(options.appRoot)); EVENTUALLY(5, LockGuard l(pool->syncher); vector processes = pool->getProcesses(false); @@ -665,39 +840,10 @@ namespace tut { process = currentSession->getProcess()->shared_from_this(); currentSession.reset(); unsigned int gen2 = process->generation; - ensure_equals(gen1+1,gen2); + ensure_equals("(2)", gen2,gen1 + 1); } - TEST_METHOD(16) { - // Test that the correct process from the pool is routed - Options options = createOptions(); - ensureMinProcesses(2); - - // async restart the group - ensure(pool->restartGroupByName(options.appRoot)); - ensureMinProcesses(1); - - /* - Imagine we have these processes (ordered from oldest to newest): - - #. PID 1 (generation A, busyness 5) - #. PID 2 (generation A, busyness 3) - #. PID 3 (generation B, busyness 1) - - The algorithm should select PID 3 - */ - - /* - Imagine we have these processes (ordered from oldest to newest): - - #. PID 1 (generation A, busyness 1) - #. PID 2 (generation B, busyness 5) - - The algorithm should select PID 1 - */ - } - - TEST_METHOD(17) { + TEST_METHOD(20) { // Test that restartGroupByName() spawns more processes to ensure // that minProcesses and other constraints are met. ensureMinProcesses(1); @@ -707,7 +853,7 @@ namespace tut { ); } - TEST_METHOD(18) { + TEST_METHOD(21) { // Test getting from an app for which minProcesses is set to 0, // and restart.txt already existed. TempDirCopy dir("stub/wsgi", "tmp.wsgi"); @@ -736,7 +882,7 @@ namespace tut { /*********** Test asyncGet() behavior on multiple Groups ***********/ - TEST_METHOD(20) { + TEST_METHOD(30) { // If the pool is full, and one tries to asyncGet() from a nonexistant group, // then it will kill the oldest idle process and spawn a new process. Options options = createOptions(); @@ -772,7 +918,7 @@ namespace tut { ensure_equals(group1->getProcessCount(), 0u); } - TEST_METHOD(21) { + TEST_METHOD(31) { // If the pool is full, and one tries to asyncGet() from a nonexistant group, // and all existing processes are non-idle, then it will // kill the oldest process and spawn a new process. @@ -808,7 +954,7 @@ namespace tut { ensure_equals(group1->getProcessCount(), 0u); } - TEST_METHOD(22) { + TEST_METHOD(32) { // Suppose the pool is at full capacity, and one tries to asyncGet() from an // existant group that does not have any processes. It should kill a process // from another group, and the request should succeed. @@ -845,7 +991,7 @@ namespace tut { ensure_equals("(4)", pool->getProcessCount(), 1u); } - TEST_METHOD(23) { + TEST_METHOD(33) { // Suppose the pool is at full capacity, and one tries to asyncGet() from an // existant group that does not have any processes, and that happens to need // restarting. It should kill a process from another group and the request @@ -888,7 +1034,7 @@ namespace tut { ensure_equals("(4)", pool->getProcessCount(), 1u); } - TEST_METHOD(24) { + TEST_METHOD(34) { // Suppose the pool is at full capacity, with two groups: // - one that is spawning a process. // - one with no processes. @@ -948,7 +1094,7 @@ namespace tut { ); } - TEST_METHOD(25) { + TEST_METHOD(35) { // Suppose the pool is at full capacity, with two groups: // - one that is spawning a process, and has a queued request. // - one with no processes. @@ -1017,7 +1163,7 @@ namespace tut { /*********** Test detachProcess() ***********/ - TEST_METHOD(30) { + TEST_METHOD(40) { // detachProcess() detaches the process from the group. The pool // will restore the minimum number of processes afterwards. Options options = createOptions(); @@ -1046,7 +1192,7 @@ namespace tut { ); } - TEST_METHOD(31) { + TEST_METHOD(41) { // If the containing group had waiters on it, and detachProcess() // detaches the only process in the group, then a new process // is automatically spawned to handle the waiters. @@ -1082,7 +1228,7 @@ namespace tut { ); } - TEST_METHOD(32) { + TEST_METHOD(42) { // If the pool had waiters on it then detachProcess() will // automatically create the Groups that were requested // by the waiters. @@ -1125,7 +1271,7 @@ namespace tut { ); } - TEST_METHOD(33) { + TEST_METHOD(43) { // A Group does not become garbage collectable // after detaching all its processes. Options options = createOptions(); @@ -1143,7 +1289,7 @@ namespace tut { ensure(!group->garbageCollectable()); } - TEST_METHOD(34) { + TEST_METHOD(44) { // When detaching a process, it waits until all sessions have // finished before telling the process to shut down. Options options = createOptions(); @@ -1172,7 +1318,7 @@ namespace tut { ); } - TEST_METHOD(35) { + TEST_METHOD(45) { // When detaching a process, it waits until the OS processes // have exited before cleaning up the in-memory data structures. Options options = createOptions(); @@ -1209,7 +1355,7 @@ namespace tut { ); } - TEST_METHOD(36) { + TEST_METHOD(46) { // Detaching a process that is already being detached, works. Options options = createOptions(); options.appGroupName = "test"; @@ -1257,7 +1403,7 @@ namespace tut { /*********** Test disabling and enabling processes ***********/ - TEST_METHOD(40) { + TEST_METHOD(50) { // Disabling a process under idle conditions should succeed immediately. ensureMinProcesses(2); vector processes = pool->getProcesses(); @@ -1275,7 +1421,7 @@ namespace tut { processes[1]->enabled, Process::ENABLED); } - TEST_METHOD(41) { + TEST_METHOD(51) { // Disabling the sole process in a group, in case the pool settings allow // spawning another process, should trigger a new process spawn. ensureMinProcesses(1); @@ -1303,7 +1449,7 @@ namespace tut { ); } - TEST_METHOD(42) { + TEST_METHOD(52) { // Disabling the sole process in a group, in case pool settings don't allow // spawning another process, should fail. pool->setMax(1); @@ -1323,7 +1469,7 @@ namespace tut { ensure_equals("(3)", pool->getProcessCount(), 1u); } - TEST_METHOD(43) { + TEST_METHOD(53) { // If there are no enabled processes in the group, then disabling should // succeed after the new process has been spawned. initPoolDebugging(); @@ -1370,7 +1516,7 @@ namespace tut { } } - TEST_METHOD(44) { + TEST_METHOD(54) { // Suppose that a previous disable command triggered a new process spawn, // and the spawn fails. Then any disabling processes should become enabled // again, and the callbacks for the previous disable commands should be called. @@ -1435,7 +1581,7 @@ namespace tut { // TODO: A disabling process becomes disabled as soon as it's done with // all its request. - TEST_METHOD(50) { + TEST_METHOD(60) { // Disabling a process that's already being disabled should result in the // callback being called after disabling is done. ensureMinProcesses(2); @@ -1459,7 +1605,7 @@ namespace tut { // TODO: Enabling a process that's disabling succeeds immediately. The disable // callbacks will be called with DR_CANCELED. - TEST_METHOD(51) { + TEST_METHOD(61) { // If the number of processes is already at maximum, then disabling // a process will cause that process to be disabled, without spawning // a new process. @@ -1484,7 +1630,7 @@ namespace tut { /*********** Other tests ***********/ - TEST_METHOD(60) { + TEST_METHOD(70) { // The pool is considered to be at full capacity if and only // if all Groups are at full capacity. Options options = createOptions(); @@ -1509,7 +1655,7 @@ namespace tut { ensure(!pool->atFullCapacity()); } - TEST_METHOD(61) { + TEST_METHOD(71) { // If the pool is at full capacity, then increasing 'max' will cause // new processes to be spawned. Any queued get requests are processed // as those new processes become available or as existing processes @@ -1532,7 +1678,7 @@ namespace tut { ensure_equals(pool->getProcessCount(), 3u); } - TEST_METHOD(62) { + TEST_METHOD(72) { // Each spawned process has a GUPID, which can be looked up // through findProcessByGupid(). Options options = createOptions(); @@ -1546,13 +1692,13 @@ namespace tut { pool->findProcessByGupid(gupid).get()); } - TEST_METHOD(63) { + TEST_METHOD(73) { // findProcessByGupid() returns a NULL pointer if there is // no matching process. ensure(pool->findProcessByGupid("none") == NULL); } - TEST_METHOD(64) { + TEST_METHOD(74) { // Test process idle cleaning. Options options = createOptions(); pool->setMaxIdleTime(50000); @@ -1578,7 +1724,7 @@ namespace tut { ); } - TEST_METHOD(65) { + TEST_METHOD(75) { // Test spawner idle cleaning. Options options = createOptions(); options.appGroupName = "test1"; @@ -1604,7 +1750,7 @@ namespace tut { ); } - TEST_METHOD(66) { + TEST_METHOD(76) { // It should restart the app if restart.txt is created or updated. TempDirCopy dir("stub/wsgi", "tmp.wsgi"); Options options = createOptions(); @@ -1641,7 +1787,7 @@ namespace tut { ensure_equals(sendRequest(options, "/"), "restarted 2"); } - TEST_METHOD(67) { + TEST_METHOD(77) { // Test spawn exceptions. TempDirCopy dir("stub/wsgi", "tmp.wsgi"); Options options = createOptions(); @@ -1670,7 +1816,7 @@ namespace tut { ensure(e->getProblemDescriptionHTML().find("Something went wrong!") != string::npos); } - TEST_METHOD(68) { + TEST_METHOD(78) { // If a process fails to spawn, then it stops trying to spawn minProcesses processes. TempDirCopy dir("stub/wsgi", "tmp.wsgi"); Options options = createOptions(); @@ -1718,7 +1864,7 @@ namespace tut { ); } - TEST_METHOD(69) { + TEST_METHOD(79) { // It removes the process from the pool if session->initiate() fails. Options options = createOptions(); options.appRoot = "stub/wsgi"; @@ -1748,7 +1894,7 @@ namespace tut { ensure_equals(pool->getProcessCount(), 0u); } - TEST_METHOD(70) { + TEST_METHOD(80) { // When a process has become idle, and there are waiters on the pool, // consider detaching it in order to satisfy a waiter. Options options1 = createOptions(); @@ -1778,7 +1924,7 @@ namespace tut { ensure_equals(group2->enabledCount, 1); } - TEST_METHOD(71) { + TEST_METHOD(81) { // A process is detached after processing maxRequests sessions. Options options = createOptions(); options.minProcesses = 0; @@ -1802,7 +1948,7 @@ namespace tut { ); } - TEST_METHOD(72) { + TEST_METHOD(82) { // If we restart while spawning is in progress, and the restart // finishes before the process is done spawning, then that // process will not be attached and the original spawn loop will @@ -1848,7 +1994,7 @@ namespace tut { ensure_equals("(4)", pool->getProcessCount(), 3u); } - TEST_METHOD(73) { + TEST_METHOD(83) { // If a get() request comes in while the restart is in progress, then // that get() request will be put into the get waiters list, which will // be processed after spawning is done. @@ -1882,7 +2028,7 @@ namespace tut { pool->getProcessCount(), 2u); } - TEST_METHOD(74) { + TEST_METHOD(84) { // If a process fails to spawn, it sends a SpawnException result to all get waiters. TempDirCopy dir("stub/wsgi", "tmp.wsgi"); chmod("tmp.wsgi", 0777); @@ -1936,7 +2082,7 @@ namespace tut { ensure(sessions.empty()); } - TEST_METHOD(75) { + TEST_METHOD(85) { // If a process fails to spawn, the existing processes // are kept alive. TempDirCopy dir("stub/wsgi", "tmp.wsgi"); @@ -1974,7 +2120,7 @@ namespace tut { } } - TEST_METHOD(76) { + TEST_METHOD(86) { // No more than maxOutOfBandWorkInstances process will be performing // out-of-band work at the same time. TempDirCopy dir("stub/wsgi", "tmp.wsgi"); @@ -2021,7 +2167,7 @@ namespace tut { debug->debugger->recv("OOBW request finished"); } - TEST_METHOD(77) { + TEST_METHOD(87) { // If the getWaitlist already has maxRequestQueueSize items, // then an exception is returned. Options options = createOptions(); @@ -2056,7 +2202,7 @@ namespace tut { ); } - TEST_METHOD(78) { + TEST_METHOD(88) { // Test restarting while a previous restart was already being finalized. // The previous finalization should abort. Options options = createOptions(); @@ -2074,7 +2220,7 @@ namespace tut { debug->debugger->recv("Restarting aborted"); } - TEST_METHOD(79) { + TEST_METHOD(89) { // Test sticky sessions. // Spawn 2 processes and get their sticky session IDs and PIDs. @@ -2135,7 +2281,7 @@ namespace tut { /*********** Test previously discovered bugs ***********/ - TEST_METHOD(85) { + TEST_METHOD(95) { // Test detaching, then restarting. This should not violate any invariants. TempDirCopy dir("stub/wsgi", "tmp.wsgi"); Options options = createOptions(); diff --git a/test/tut/tut.h b/test/tut/tut.h index 016117c065..80a8cb1db9 100644 --- a/test/tut/tut.h +++ b/test/tut/tut.h @@ -837,6 +837,30 @@ void ensure_not_equals(const char* msg, const Q& a, const T& b) } } +/** + * Tests that A > B. + * Throws if false. + * + * NB: both T and Q must have operator << defined somewhere, or + * client code will not compile at all! + */ +template +void ensure_gt(const char* msg, const Q& a, const T& b) +{ + if (!(a > b)) + { + std::stringstream ss; + ss << (msg ? msg : "") + << (msg ? ":" : "") + << " expected '" + << a + << "' to be greater than '" + << b + << '\''; + throw failure(ss.str().c_str()); + } +} + /** * Tests two objects for being at most in given distance one from another. * Borders are excluded. From 8a5ebc3fd0385a3561ed9a7596221f8bd936816d Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 11 Nov 2024 09:17:11 -0700 Subject: [PATCH 26/29] address comments about only finding routeable processes --- .../Group/ProcessListManagement.cpp | 83 +------------------ .../Group/SessionManagement.cpp | 16 +--- 2 files changed, 2 insertions(+), 97 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index 8a5cc05034..120feb5eb1 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -75,21 +75,12 @@ Group::findProcessWithStickySessionId(unsigned int id) const { Process * Group::findBestProcessPreferringStickySessionId(unsigned int id) const { Process *bestProcess = nullptr; - Process *fallbackProcess = nullptr; ProcessList::const_iterator it; ProcessList::const_iterator end = enabledProcesses.end(); for (it = enabledProcesses.begin(); it != end; it++) { Process *process = it->get(); if (process->getStickySessionId() == id) { return process; - } else if (process->isTotallyBusy() && bestProcess == nullptr) { - if (fallbackProcess == nullptr || - process->generation > fallbackProcess->generation || - (process->generation == fallbackProcess->generation && process->spawnStartTime < fallbackProcess->spawnStartTime) || - (process->generation == fallbackProcess->generation && process->spawnStartTime == fallbackProcess->spawnStartTime && process->busyness() < fallbackProcess->busyness()) - ) { - fallbackProcess = process; - } } else if (!process->isTotallyBusy() && (bestProcess == nullptr || process->generation > bestProcess->generation || (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) || @@ -98,9 +89,6 @@ Group::findBestProcessPreferringStickySessionId(unsigned int id) const { bestProcess = process; } } - if (bestProcess == nullptr) { - return fallbackProcess; - } return bestProcess; } @@ -120,21 +108,12 @@ Group::findBestProcess(const ProcessList &processes) const { } Process *bestProcess = nullptr; - Process *fallbackProcess = nullptr; ProcessList::const_iterator it; ProcessList::const_iterator end = processes.end(); for (it = processes.begin(); it != end; it++) { Process *process = (*it).get(); - if (process->isTotallyBusy() && bestProcess == nullptr) { - if (fallbackProcess == nullptr || - process->generation > fallbackProcess->generation || - (process->generation == fallbackProcess->generation && process->spawnStartTime < fallbackProcess->spawnStartTime) || - (process->generation == fallbackProcess->generation && process->spawnStartTime == fallbackProcess->spawnStartTime && process->busyness() < fallbackProcess->busyness()) - ) { - fallbackProcess = process; - } - } else if (!process->isTotallyBusy() && (bestProcess == nullptr || + if (!process->isTotallyBusy() && (bestProcess == nullptr || process->generation > bestProcess->generation || (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) || (process->generation == bestProcess->generation && process->spawnStartTime == bestProcess->spawnStartTime && process->busyness() < bestProcess->busyness()) @@ -142,66 +121,6 @@ Group::findBestProcess(const ProcessList &processes) const { bestProcess = process; } } - if (bestProcess == nullptr) { - return fallbackProcess; - } - return bestProcess; -} - -/** - * Cache-optimized version of `findBestProcess()` for the common case. - * See `findBestProcess()` for the general contract. - */ -Process * -Group::findBestEnabledProcess() const { - if (enabledProcesses.empty()) { - return nullptr; - } - - Process *bestProcess = nullptr; - unsigned long long bestProcessStartTime = 0; - unsigned int bestProcessGen = 0; - int bestProcessBusyness = 0; - - Process *fallbackProcess = nullptr; - unsigned long long fallbackProcessStartTime = 0; - unsigned int fallbackProcessGen = 0; - int fallbackProcessBusyness = 0; - - unsigned int size = enabledProcessBusynessLevels.size(); - const int *enabledProcessBusynessLevels = &this->enabledProcessBusynessLevels[0]; - - for (unsigned int i = 0; i < size; i++) { - Process *process = enabledProcesses.at(i).get(); - unsigned int gen = process->generation; - unsigned long long startTime = process->spawnStartTime; - int busyness = enabledProcessBusynessLevels[i]; - bool totallyBusy = process->isTotallyBusy(); - if (totallyBusy && bestProcess == nullptr) { - if (fallbackProcess == nullptr || - gen > fallbackProcess->generation || - (gen == fallbackProcessGen && startTime < fallbackProcessStartTime) || - (gen == fallbackProcessGen && startTime == fallbackProcessStartTime && busyness < fallbackProcessBusyness) - ) { - fallbackProcess = process; - fallbackProcessGen = gen; - fallbackProcessBusyness = busyness; - fallbackProcessStartTime = startTime; - } - } else if (!totallyBusy && (bestProcess == nullptr || - gen > bestProcess->generation || - (gen == bestProcessGen && startTime < bestProcessStartTime) || - (gen == bestProcessGen && startTime == bestProcessStartTime && busyness < bestProcessBusyness) - )) { - bestProcess = process; - bestProcessGen = gen; - bestProcessBusyness = busyness; - bestProcessStartTime = startTime; - } - } - if (bestProcess == nullptr) { - return fallbackProcess; - } return bestProcess; } diff --git a/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp b/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp index 1c5ee53512..8bdccbbc03 100644 --- a/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp @@ -65,15 +65,8 @@ Group::RouteResult Group::route(const Options &options) const { if (OXT_LIKELY(enabledCount > 0)) { if (options.stickySessionId == 0) { - Process *process = findBestEnabledProcess(); + Process *process = findBestProcess(enabledProcesses); if (process != nullptr) { - // TODO: remove this part when findBestEnabledProcess() has been - // modified to adhere to the new contract with postcondition - // `result != nullptr || result.canBeRoutedTo()` - if (!process->canBeRoutedTo()) { - return RouteResult(nullptr, false); - } - assert(process->canBeRoutedTo()); return RouteResult(process); } else { @@ -95,13 +88,6 @@ Group::route(const Options &options) const { } else { Process *process = findBestProcess(disablingProcesses); if (process != nullptr) { - // TODO: remove this part when findBestProcess() has been - // modified to adhere to the new contract with postcondition - // `result != nullptr || result.canBeRoutedTo()` - if (!process->canBeRoutedTo()) { - return RouteResult(nullptr, false); - } - assert(process->canBeRoutedTo()); return RouteResult(process); } else { From 170c628ad1063c7355e8cb2385fc7f56843c69cf Mon Sep 17 00:00:00 2001 From: Hongli Lai Date: Sun, 10 Nov 2024 16:53:15 +0100 Subject: [PATCH 27/29] fixes --- .../Group/ProcessListManagement.cpp | 36 ++++++++++++------- .../Group/SessionManagement.cpp | 3 +- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp index 120feb5eb1..5c17841a59 100644 --- a/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/ProcessListManagement.cpp @@ -70,25 +70,30 @@ Group::findProcessWithStickySessionId(unsigned int id) const { * * - If the process with the given sticky session ID exists, then always * returns that process. Meaning that this process could be `!canBeRoutedTo()`. - * - If there is no process that be routed to, then returns nullptr. + * - If there is no process that can be routed to, then returns nullptr. */ Process * Group::findBestProcessPreferringStickySessionId(unsigned int id) const { Process *bestProcess = nullptr; ProcessList::const_iterator it; ProcessList::const_iterator end = enabledProcesses.end(); + for (it = enabledProcesses.begin(); it != end; it++) { Process *process = it->get(); if (process->getStickySessionId() == id) { return process; - } else if (!process->isTotallyBusy() && (bestProcess == nullptr || - process->generation > bestProcess->generation || - (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) || - (process->generation == bestProcess->generation && process->spawnStartTime == bestProcess->spawnStartTime && process->busyness() < bestProcess->busyness()) - )) { + } else if (!process->isTotallyBusy() + && ( + bestProcess == nullptr + || process->generation > bestProcess->generation + || (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) + || (process->generation == bestProcess->generation && process->spawnStartTime == bestProcess->spawnStartTime && process->busyness() < bestProcess->busyness()) + ) + ) { bestProcess = process; } } + return bestProcess; } @@ -97,7 +102,7 @@ Group::findBestProcessPreferringStickySessionId(unsigned int id) const { * At the moment, "best" is defined as the process with the highest generation, * lowest start time, and lowest busyness, in that order of priority. * - * If there is no process that be routed to, then returns nullptr. + * If there is no process that can be routed to, then returns nullptr. * * @post result != nullptr || result.canBeRoutedTo() */ @@ -110,17 +115,22 @@ Group::findBestProcess(const ProcessList &processes) const { Process *bestProcess = nullptr; ProcessList::const_iterator it; ProcessList::const_iterator end = processes.end(); + for (it = processes.begin(); it != end; it++) { - Process *process = (*it).get(); + Process *process = it->get(); - if (!process->isTotallyBusy() && (bestProcess == nullptr || - process->generation > bestProcess->generation || - (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) || - (process->generation == bestProcess->generation && process->spawnStartTime == bestProcess->spawnStartTime && process->busyness() < bestProcess->busyness()) - )) { + if (!process->isTotallyBusy() + && ( + bestProcess == nullptr + || process->generation > bestProcess->generation + || (process->generation == bestProcess->generation && process->spawnStartTime < bestProcess->spawnStartTime) + || (process->generation == bestProcess->generation && process->spawnStartTime == bestProcess->spawnStartTime && process->busyness() < bestProcess->busyness()) + ) + ) { bestProcess = process; } } + return bestProcess; } diff --git a/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp b/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp index 8bdccbbc03..0a7af88e0e 100644 --- a/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp +++ b/src/agent/Core/ApplicationPool/Group/SessionManagement.cpp @@ -314,8 +314,7 @@ Group::get(const Options &newOptions, const GetCallback &callback, if (disablingCount > 0 && !restarting()) { Process *process = findBestProcess(disablingProcesses); - assert(process != NULL); - if (!process->isTotallyBusy()) { + if (process != nullptr && !process->isTotallyBusy()) { return newSession(process, newOptions.currentTime); } } From f06c4f7cfc3c41ee2e85087f50411656812cfcfc Mon Sep 17 00:00:00 2001 From: Camden Narzt Date: Mon, 18 Nov 2024 15:33:07 -0700 Subject: [PATCH 28/29] fixup changelog [ci skip] [ci:skip] --- CHANGELOG | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a517b8d252..fa87c3550b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,7 +1,6 @@ Release 6.0.24 (Not yet released) ------------- - * Update the order Passenger routes requests to app processes. Processes are now chosen based on being in the latest generation (Enterprise), then by newest process, then by oldest, then by -busyness. Closes GH-2551. + * Update the order Passenger routes requests to app processes. Processes are now chosen based on being in the latest generation (Enterprise), then by oldest process, then by (least) busyness. Closes GH-2551. * Fix a regression from 6.0.10 where running `passenger-config system-properties` would throw an error. Closes GH-2565. * [Enterprise] Fix a memory corruption-related crash that could occur during rolling restarting. * [Ubuntu] Add packages for Ubuntu 24.10 "oracular". From 7a59e7b566f4887dffb0a7983e4d8e3ad6c2bf06 Mon Sep 17 00:00:00 2001 From: Hongli Lai Date: Thu, 21 Nov 2024 17:08:57 +0100 Subject: [PATCH 29/29] Update changelog [ci skip] --- CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index fa87c3550b..3672ced866 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,6 @@ Release 6.0.24 (Not yet released) ------------- - * Update the order Passenger routes requests to app processes. Processes are now chosen based on being in the latest generation (Enterprise), then by oldest process, then by (least) busyness. Closes GH-2551. + * [Enterprise] Smarter rolling restarts for better performance and reliability. We changed the way we route requests. Instead of picking the least-busy process, we now first prioritize new processes first. During a rolling restart, this new behavior leads to more efficient utilization of application caches, faster validation of new rollouts, and faster recovery from problematic deployments. Closes GH-2551. * Fix a regression from 6.0.10 where running `passenger-config system-properties` would throw an error. Closes GH-2565. * [Enterprise] Fix a memory corruption-related crash that could occur during rolling restarting. * [Ubuntu] Add packages for Ubuntu 24.10 "oracular".