Permalink
Browse files

Fix some bugs in the enable/disable process functionality.

  • Loading branch information...
1 parent 0c1937a commit 19f23d98e958b9d8367b4f8c173ba5f3766df884 @FooBarWidget FooBarWidget committed Oct 31, 2012
Showing with 79 additions and 28 deletions.
  1. +30 −7 ext/common/ApplicationPool2/Group.h
  2. +9 −7 ext/common/ApplicationPool2/Pool.h
  3. +40 −14 test/cxx/ApplicationPool2/PoolTest.cpp
@@ -41,6 +41,7 @@
#include <ApplicationPool2/Spawner.h>
#include <ApplicationPool2/Process.h>
#include <ApplicationPool2/Options.h>
+#include <Utils.h>
#include <Utils/CachedFileStat.hpp>
#include <Utils/FileChangeChecker.h>
#include <Utils/SmallVector.h>
@@ -140,6 +141,26 @@ class Group: public enable_shared_from_this<Group> {
assert((int) enabledProcesses.size() == enabledCount);
assert((int) disablingProcesses.size() == disablingCount);
assert((int) disabledProcesses.size() == disabledCount);
+
+ ProcessList::const_iterator it, end;
+
+ end = enabledProcesses.end();
+ for (it = enabledProcesses.begin(); it != end; it++) {
+ const ProcessPtr &process = *it;
+ assert(process->enabled == Process::ENABLED);
+ }
+
+ end = disablingProcesses.end();
+ for (it = disablingProcesses.begin(); it != end; it++) {
+ const ProcessPtr &process = *it;
+ assert(process->enabled == Process::DISABLING);
+ }
+
+ end = disabledProcesses.end();
+ for (it = disabledProcesses.begin(); it != end; it++) {
+ const ProcessPtr &process = *it;
+ assert(process->enabled == Process::DISABLED);
+ }
}
void resetOptions(const Options &newOptions) {
@@ -238,6 +259,7 @@ class Group: public enable_shared_from_this<Group> {
process->enabled = Process::DISABLING;
disablingCount++;
} else if (&destination == &disabledProcesses) {
+ assert(process->sessions == 0);
process->enabled = Process::DISABLED;
disabledCount++;
} else {
@@ -568,8 +590,8 @@ class Group: public enable_shared_from_this<Group> {
process->setGroup(shared_from_this());
addProcessToList(process, enabledProcesses);
- /* Now that there are enough resources all process in 'disableWaitlist'
- * can be disabled.
+ /* 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());
@@ -579,7 +601,7 @@ class Group: public enable_shared_from_this<Group> {
// The same process can appear multiple times in disableWaitlist.
assert(process2->enabled == Process::DISABLING
|| process2->enabled == Process::DISABLED);
- if (process2->enabled == Process::DISABLING) {
+ if (process2->enabled == Process::DISABLING && process2->sessions == 0) {
removeProcessFromList(process2, disablingProcesses);
addProcessToList(process2, disabledProcesses);
}
@@ -664,8 +686,8 @@ 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) {
- assert(enabledCount > 0);
- if (enabledCount == 1) {
+ 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
@@ -675,10 +697,11 @@ class Group: public enable_shared_from_this<Group> {
removeProcessFromList(process, enabledProcesses);
addProcessToList(process, disablingProcesses);
disableWaitlist.push_back(DisableWaiter(process, callback));
- spawn();
+ if (enabledCount == 0) {
+ spawn();
+ }
return DR_DEFERRED;
} else {
- assert(enabledCount > 1);
removeProcessFromList(process, enabledProcesses);
addProcessToList(process, disabledProcesses);
return DR_SUCCESS;
@@ -405,7 +405,7 @@ class Pool: public enable_shared_from_this<Pool> {
};
static void syncDisableProcessCallback(const ProcessPtr &process, DisableResult result,
- DisableWaitTicket *ticket)
+ shared_ptr<DisableWaitTicket> ticket)
{
ScopedLock l(ticket->syncher);
ticket->done = true;
@@ -894,6 +894,7 @@ class Pool: public enable_shared_from_this<Pool> {
}
}
+ // TODO: 'ticket' should be a shared_ptr for interruption-safety.
SessionPtr get(const Options &options, Ticket *ticket) {
ticket->session.reset();
ticket->exception.reset();
@@ -1140,16 +1141,17 @@ class Pool: public enable_shared_from_this<Pool> {
ScopedLock l(syncher);
ProcessPtr process = findProcessByGupid(gupid, false);
GroupPtr group = process->getGroup();
- DisableWaitTicket ticket;
+ // Must be a shared_ptr to be interruption-safe.
+ shared_ptr<DisableWaitTicket> ticket = make_shared<DisableWaitTicket>();
DisableResult result = group->disable(process,
- boost::bind(syncDisableProcessCallback, _1, _2, &ticket));
+ boost::bind(syncDisableProcessCallback, _1, _2, ticket));
if (result == DR_DEFERRED) {
l.unlock();
- ScopedLock l2(ticket.syncher);
- while (!ticket.done) {
- ticket.cond.wait(l2);
+ ScopedLock l2(ticket->syncher);
+ while (!ticket->done) {
+ ticket->cond.wait(l2);
}
- return ticket.result;
+ return ticket->result;
} else {
return result;
}
@@ -139,6 +139,24 @@ namespace tut {
);
return body;
}
+
+ Options ensureMinProcesses(unsigned int n) {
+ Options options = createOptions();
+ options.minProcesses = 2;
+ pool->asyncGet(options, callback);
+ EVENTUALLY(5,
+ result = number == 1;
+ );
+ EVENTUALLY(5,
+ result = pool->getProcessCount() == n;
+ );
+ currentSession.reset();
+ return options;
+ }
+
+ void disableProcess(ProcessPtr process, AtomicInt *result) {
+ *result = (int) pool->disableProcess(process->gupid);
+ }
};
DEFINE_TEST_GROUP_WITH_LIMIT(ApplicationPool2_PoolTest, 100);
@@ -713,18 +731,9 @@ namespace tut {
TEST_METHOD(40) {
// Disabling a process under idle conditions should succeed immediately.
- Options options = createOptions();
- options.minProcesses = 2;
- pool->asyncGet(options, callback);
- EVENTUALLY(5,
- result = number == 1;
- );
- EVENTUALLY(5,
- result = pool->getProcessCount() == 2;
- );
-
+ ensureMinProcesses(2);
vector<ProcessPtr> processes = pool->getProcesses();
- ensure_equals("Disabling succeeds immediately",
+ ensure_equals("Disabling succeeds",
pool->disableProcess(processes[0]->gupid), DR_SUCCESS);
LockGuard l(pool->syncher);
@@ -741,7 +750,6 @@ namespace tut {
// Disabling the sole process in a group should trigger a new process spawn.
// 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,
// and the spawn fails. Then any disabling processes should become enabled
// again, and the callbacks for the previous disable commands should be called.
@@ -760,8 +768,26 @@ namespace tut {
// A disabling process becomes disabled as soon as it's done with
// all its request.
- // Disabling a process that's already being disabled should result in the
- // callback being called after disabling is done.
+ TEST_METHOD(41) {
+ // Disabling a process that's already being disabled should result in the
+ // callback being called after disabling is done.
+ ensureMinProcesses(2);
+ Options options = createOptions();
+ Ticket ticket;
+ SessionPtr session = pool->get(options, &ticket);
+
+ AtomicInt code = -1;
+ TempThread thr(boost::bind(&ApplicationPool2_PoolTest::disableProcess,
+ this, session->getProcess(), &code));
+ SHOULD_NEVER_HAPPEN(100,
+ result = code != -1;
+ );
+ session.reset();
+ EVENTUALLY(1,
+ result = code != -1;
+ );
+ ensure_equals(code, (int) DR_SUCCESS);
+ }
// Enabling a process that's disabled succeeds immediately.
// Enabling a process that's disabling succeeds immediately. The disable

0 comments on commit 19f23d9

Please sign in to comment.