Permalink
Browse files

Fix more bugs

  • Loading branch information...
1 parent f9db140 commit 23ecf4eddf37948095cb8a5df1c64d74561c80a8 @FooBarWidget FooBarWidget committed Nov 1, 2012
@@ -350,9 +350,11 @@ class Group: public enable_shared_from_this<Group> {
for (it = disableWaitlist.begin(); it != end; it++) {
const DisableWaiter &waiter = *it;
const ProcessPtr process = waiter.process;
- assert(process->enabled == Process::DISABLING);
- removeProcessFromList(process, disablingProcesses);
- addProcessToList(process, enabledProcesses);
+ // A process can appear multiple times in disableWaitlist.
+ if (process->enabled == Process::DISABLING) {
+ removeProcessFromList(process, disablingProcesses);
+ addProcessToList(process, enabledProcesses);
+ }
}
clearDisableWaitlist(DR_ERROR, postLockActions);
}
@@ -588,26 +590,31 @@ class Group: public enable_shared_from_this<Group> {
void attach(const ProcessPtr &process, vector<Callback> &postLockActions) {
assert(process->getGroup() == NULL);
process->setGroup(shared_from_this());
+ P_DEBUG("Attaching process " << process->inspect());
addProcessToList(process, enabledProcesses);
/* Now that there are enough resources, relevant processes in
* 'disableWaitlist' can be disabled.
*/
deque<DisableWaiter>::const_iterator it, end = disableWaitlist.end();
- postLockActions.reserve(postLockActions.size() + disableWaitlist.size());
+ deque<DisableWaiter> newDisableWaitlist;
for (it = disableWaitlist.begin(); it != end; it++) {
const DisableWaiter &waiter = *it;
const ProcessPtr process2 = waiter.process;
// The same process can appear multiple times in disableWaitlist.
assert(process2->enabled == Process::DISABLING
|| process2->enabled == Process::DISABLED);
if (process2->enabled == Process::DISABLING && process2->sessions == 0) {
+ P_DEBUG("Disabling DISABLING process " << process2->inspect() <<
+ "; disable command succeeded immediately");
removeProcessFromList(process2, disablingProcesses);
addProcessToList(process2, disabledProcesses);
+ postLockActions.push_back(boost::bind(waiter.callback, process2, DR_SUCCESS));
+ } else {
+ newDisableWaitlist.push_back(waiter);
}
- postLockActions.push_back(boost::bind(waiter.callback, process2, DR_SUCCESS));
}
- disableWaitlist.clear();
+ disableWaitlist = newDisableWaitlist;
}
/**
@@ -669,12 +676,16 @@ class Group: public enable_shared_from_this<Group> {
void enable(const ProcessPtr &process, vector<Callback> &postLockActions) {
assert(process->getGroup().get() == this);
if (process->enabled == Process::DISABLING) {
+ P_DEBUG("Enabling DISABLING process " << process->inspect());
removeProcessFromList(process, disablingProcesses);
addProcessToList(process, enabledProcesses);
removeFromDisableWaitlist(process, DR_CANCELED, postLockActions);
} else if (process->enabled == Process::DISABLED) {
+ P_DEBUG("Enabling DISABLED process " << process->inspect());
removeProcessFromList(process, disabledProcesses);
addProcessToList(process, enabledProcesses);
+ } else {
+ P_DEBUG("Enabling ENABLED process " << process->inspect());
}
}
@@ -686,32 +697,41 @@ class Group: public enable_shared_from_this<Group> {
DisableResult disable(const ProcessPtr &process, const DisableCallback &callback) {
assert(process->getGroup().get() == this);
if (process->enabled == Process::ENABLED) {
+ P_DEBUG("Disabling ENABLED process " << process->inspect() <<
+ "; enabledCount=" << enabledCount << ", process.sessions=" << process->sessions);
assert(enabledCount >= 0);
if (enabledCount <= 1 || process->sessions > 0) {
- /* All processes are going to be disabled, so in order
- * to avoid blocking requests we first spawn a new process
- * and disable this process after the other one is done
- * spawning. We do this irregardless of resource limits
- * because this is an exceptional situation.
- */
removeProcessFromList(process, enabledProcesses);
addProcessToList(process, disablingProcesses);
disableWaitlist.push_back(DisableWaiter(process, callback));
if (enabledCount == 0) {
+ /* All processes are going to be disabled, so in order
+ * to avoid blocking requests we first spawn a new process
+ * and disable this process after the other one is done
+ * spawning. We do this irregardless of resource limits
+ * because this is an exceptional situation.
+ */
+ P_DEBUG("Spawning a new process to avoid the disable action from blocking requests");
spawn();
}
+ P_DEBUG("Deferring disable command completion");
return DR_DEFERRED;
} else {
removeProcessFromList(process, enabledProcesses);
addProcessToList(process, disabledProcesses);
+ P_DEBUG("Disable command succeeded immediately");
return DR_SUCCESS;
}
} else if (process->enabled == Process::DISABLING) {
assert(disablingCount > 0);
disableWaitlist.push_back(DisableWaiter(process, callback));
+ P_DEBUG("Disabling DISABLING process " << process->inspect() <<
+ name << "; command queued, deferring disable command completion");
return DR_DEFERRED;
} else {
assert(disabledCount > 0);
+ P_DEBUG("Disabling DISABLED process " << process->inspect() <<
+ name << "; disable command succeeded immediately");
return DR_NOOP;
}
}
@@ -392,17 +392,17 @@ Group::spawnThreadRealMain(const SpawnerPtr &spawner, const Options &options) {
} else {
assignSessionsToGetWaiters(actions);
}
- P_DEBUG("Attached process " << process->inspect() <<
- " to group " << name <<
- ": new process count = " << enabledCount <<
+ P_DEBUG("New process count = " << enabledCount <<
", remaining get waiters = " << getWaitlist.size());
} else {
// TODO: sure this is the best thing? if there are
// processes currently alive we should just use them.
- P_DEBUG("Could not spawn process appRoot=" << name <<
+ P_ERROR("Could not spawn process for group " << name <<
": " << exception->what());
+ if (enabledCount == 0) {
+ enableAllDisablingProcesses(actions);
+ }
assignExceptionToGetWaiters(exception, actions);
- enableAllDisablingProcesses(actions);
pool->assignSessionsToGetWaiters(actions);
done = true;
}
@@ -35,6 +35,7 @@
#include <boost/shared_ptr.hpp>
#include <boost/make_shared.hpp>
#include <boost/function.hpp>
+#include <boost/foreach.hpp>
#include <oxt/dynamic_thread_group.hpp>
#include <oxt/backtrace.hpp>
#include <ApplicationPool2/Common.h>
@@ -1145,6 +1146,8 @@ class Pool: public enable_shared_from_this<Pool> {
shared_ptr<DisableWaitTicket> ticket = make_shared<DisableWaitTicket>();
DisableResult result = group->disable(process,
boost::bind(syncDisableProcessCallback, _1, _2, ticket));
+ group->verifyInvariants();
+ group->verifyExpensiveInvariants();
if (result == DR_DEFERRED) {
l.unlock();
ScopedLock l2(ticket->syncher);
@@ -1157,6 +1160,22 @@ class Pool: public enable_shared_from_this<Pool> {
}
}
+ /**
+ * Checks whether at least one process is being spawned.
+ */
+ bool isSpawning(bool lock = true) const {
+ DynamicScopedLock l(syncher, lock);
+ SuperGroupMap::const_iterator it, end = superGroups.end();
+ for (it = superGroups.begin(); it != end; it++) {
+ foreach (GroupPtr group, it->second->groups) {
+ if (group->spawning()) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
string inspect(const InspectOptions &options = InspectOptions(), bool lock = true) const {
DynamicScopedLock l(syncher, lock);
stringstream result;
View
@@ -47,6 +47,9 @@ namespace Passenger {
using namespace std;
using namespace boost;
+#define foreach BOOST_FOREACH
+#define reverse_foreach BOOST_REVERSE_FOREACH
+
static const uid_t USER_NOT_GIVEN = (uid_t) -1;
static const gid_t GROUP_NOT_GIVEN = (gid_t) -1;
@@ -140,9 +140,10 @@ namespace tut {
return body;
}
+ // Ensure that n processes exist.
Options ensureMinProcesses(unsigned int n) {
Options options = createOptions();
- options.minProcesses = 2;
+ options.minProcesses = n;
pool->asyncGet(options, callback);
EVENTUALLY(5,
result = number == 1;
@@ -747,7 +748,33 @@ namespace tut {
processes[1]->enabled, Process::ENABLED);
}
- // Disabling the sole process in a group should trigger a new process spawn.
+ TEST_METHOD(41) {
+ // Disabling the sole process in a group should trigger a new process spawn.
+ ensureMinProcesses(1);
+ Options options = createOptions();
+ Ticket ticket;
+ SessionPtr session = pool->get(options, &ticket);
+
+ ensure_equals(pool->getProcessCount(), 1u);
+ ensure(!pool->isSpawning());
+
+ GroupPtr group = session->getProcess()->getGroup();
+ dynamic_pointer_cast<DummySpawner>(group->spawner)->spawnTime = 60000;
+ AtomicInt code = -1;
+ TempThread thr(boost::bind(&ApplicationPool2_PoolTest::disableProcess,
+ this, session->getProcess(), &code));
+ EVENTUALLY2(30, 1,
+ result = pool->isSpawning();
+ );
+ EVENTUALLY(1,
+ result = pool->getProcessCount() == 2u;
+ );
+ session.reset();
+ EVENTUALLY(1,
+ result = code == (int) DR_SUCCESS;
+ );
+ }
+
// If there are no enabled processes in the group, then disabling should
// succeed after the new process has been spawned.
// Suppose that a previous disable command triggered a new process spawn,
@@ -768,7 +795,7 @@ namespace tut {
// A disabling process becomes disabled as soon as it's done with
// all its request.
- TEST_METHOD(41) {
+ TEST_METHOD(50) {
// Disabling a process that's already being disabled should result in the
// callback being called after disabling is done.
ensureMinProcesses(2);
@@ -968,6 +995,8 @@ namespace tut {
"import sys\n"
"sys.stderr.write('Something went wrong!')\n"
"exit(1)\n");
+
+ setLogLevel(-2);
pool->asyncGet(options, callback);
EVENTUALLY(5,
result = number == 1;
@@ -1005,6 +1034,7 @@ namespace tut {
" sys.stderr.write('Something went wrong!')\n"
" exit(1)\n");
+ setLogLevel(-2);
pool->asyncGet(options, callback);
EVENTUALLY(5,
result = number == 1;
@@ -1214,6 +1244,7 @@ namespace tut {
"exit(1)\n");
retainSessions = true;
+ setLogLevel(-2);
pool->asyncGet(options, callback);
pool->asyncGet(options, callback);
pool->asyncGet(options, callback);
@@ -1260,6 +1291,7 @@ namespace tut {
"exit(1)\n");
try {
Ticket ticket;
+ setLogLevel(-2);
currentSession = pool->get(options, &ticket);
fail("SpawnException expected");
} catch (const SpawnException &) {

0 comments on commit 23ecf4e

Please sign in to comment.