feature: out of band requests #67

Merged
merged 2 commits into from Nov 19, 2012

Projects

None yet

2 participants

@pkmiec

continuing from #66

  • converted client->session->oobwRequested to a thread-safe method on group
  • reverted readAll when making oobw request
  • fixed require in out_of_band_gc.rb
  • continue to assign sessions to waitlist whenever possible
  • improved request handler test

Let me know if / where you'd like me to add docs.

@pkmiec

BTW, starting standalone without the PASSENGER_DEBUG set,

/Users/pkmiec/.rvm/gems/ree-1.8.7-2012.02/gems/passenger-3.9.1.beta/lib/phusion_passenger/standalone/command.rb:191:in `write_nginx_config_file': undefined local variable or method `passenger_support_files_dir' for #<PhusionPassenger::Standalone::StartCommand:0x10e685ba0> (NameError)
@FooBarWidget
Phusion B.V. member

Yep, the Passenger Standalone bug is a different one which I'll fix seperately.

I'm mostly done with the enable/disable process stuff. Could you rebase your patch against HEAD?

@FooBarWidget FooBarWidget commented on an outdated diff Nov 2, 2012
ext/common/ApplicationPool2/Implementation.cpp
@@ -318,6 +324,175 @@
}
}
+void
+Group::requestOOBW(const ProcessPtr &process) {
+ // Standard resource management boilerplate stuff...
+ PoolPtr pool = getPool();
+ if (OXT_UNLIKELY(pool == NULL)) {
+ return;
+ }
+ unique_lock<boost::mutex> lock(pool->syncher);
+ pool = getPool();
+ if (OXT_UNLIKELY(pool == NULL)) {
@FooBarWidget
FooBarWidget Nov 2, 2012

I found a nasty race condition in my code today, and I see you're suffering from it too. You should change this check here to: OXT_UNLIKELY(pool == NULL || process->detached())

@FooBarWidget FooBarWidget commented on an outdated diff Nov 2, 2012
ext/common/ApplicationPool2/Implementation.cpp
+ process->oobwRequested = true;
+}
+
+// The 'self' parameter is for keeping the current Group object alive
+void
+Group::lockAndAsyncOOBWRequestIfNeeded(GroupPtr self, const ProcessPtr &process) {
+ TRACE_POINT();
+
+ // Standard resource management boilerplate stuff...
+ PoolPtr pool = getPool();
+ if (OXT_UNLIKELY(pool == NULL)) {
+ return;
+ }
+ unique_lock<boost::mutex> lock(pool->syncher);
+ pool = getPool();
+ if (OXT_UNLIKELY(pool == NULL)) {
@FooBarWidget
FooBarWidget Nov 2, 2012

I found a nasty race condition in my code today, and I see you're suffering from it too. You should change this check here to: OXT_UNLIKELY(pool == NULL || process->detached())

@FooBarWidget FooBarWidget commented on the diff Nov 2, 2012
ext/common/ApplicationPool2/Implementation.cpp
+void
+Group::spawnThreadOOBWRequest(GroupPtr self, const ProcessPtr &process) {
+ TRACE_POINT();
+
+ Socket *socket;
+ Connection connection;
+
+ {
+ // Standard resource management boilerplate stuff...
+ PoolPtr pool = getPool();
+ if (OXT_UNLIKELY(pool == NULL)) {
+ return;
+ }
+ unique_lock<boost::mutex> lock(pool->syncher);
+ pool = getPool();
+ if (OXT_UNLIKELY(pool == NULL)) {
@FooBarWidget
FooBarWidget Nov 2, 2012

Here too: OXT_UNLIKELY(pool == NULL || process->detached())

@pkmiec

Thanks ... back on this again today.

@pkmiec

While working on the above feedback, I noticed two problems with master,

In attach(), where the same process could be in the disableWaitlist ... however, the first waiter for some process to call addProcessToList changes the enabled state of that process. This causes further waiters for that same process to fall into the else clause.

In onSessionClose(), you have,

        removeProcessFromList(process, disablingProcesses);
        addProcessToList(process, enabledProcesses);
        removeFromDisableWaitlist(process, DR_SUCCESS, actions);

where I think you meant,

        removeProcessFromList(process, disablingProcesses);
        addProcessToList(process, **disabledProcesses**);
        removeFromDisableWaitlist(process, DR_SUCCESS, actions);
@FooBarWidget
Phusion B.V. member

Ah yes, that code in onSessionClose() was a typo. I've fixed that in the bugfixes I committed this weekend.

I don't understand your comment about attach(). You're only supposed to call attach() on a process that isn't yet in the Group, so it couldn't possibly be in the disableWaitlist.

@pkmiec

I don't mean that the process being attached is in the disableWaitlist, but rather the disableWaitlist may contain multiple waiters and some of those waiters may point to the same process. So this snippet,

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);
}

should be more like this,

if (process2->sessions == 0) {
  if (process2->enabled == Process::DISABLING) {
    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);
}

@FooBarWidget FooBarWidget merged commit a92312e into phusion:master Nov 19, 2012
@FooBarWidget
Phusion B.V. member

Thanks. I've merged your patches with a few minor changes.

@FooBarWidget
Phusion B.V. member

Everything's looking good so far. If no unexpected problems are found this will be part of beta 2.

How did you like contributing to Passenger 4's codebase? Is all the code clearly understandable, readable, etc?

@pkmiec

Awesome!

First, I really appreciate the code base having tests (cxx, ruby, integration). I feel much more confident making changes knowing that all the existing tests still pass. Second, c++ is not my primary language these days and I appreciate your help and patience.

I was able to read through the code and figure out the places I needed to modify fairly quickly. There was enough comments and examples of similar looking code (i.e. spawning a thread, acquiring lock, passing calbacks) for me to piece together what I needed to do.

The parts I had trouble with was mainly related to getting all the details of locking correct (e.g. process is not detached). I still think it would be good idea to invest some time into a macro or a function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment