On 31/08/11 18:02, Alex Rousskov wrote:
On 08/13/2011 05:59 AM, Amos Jeffries wrote:
This migrates Cache Manager to using the new IPC subscription API for
handling message events.
To do this the CacheManager is converted to an AsyncJob which runs from
the point of first action registration until shutdown. The existing
event handlers are move into its scope as-is.
=== modified file 'src/CacheManager.h'
+class CacheManager : public AsyncJob
If CacheManager is a job now, we should not communicate with it using
Instance() and most public methods because it may not be there (or may
not be ready to talk) when we want to talk to it. Jobs should
communicate using async calls.
Please note that I do think CacheManager should be a job [eventually],
but we should change its usage accordingly or, at the very least,
provide temporary controls that bad things do not happen (e.g.,
CacheManager::Instance() returning a stale pointer to the job that has
been destroyed).
=== modified file 'src/cache_manager.cc'
+void
+CacheManager::swanSong()
+{
+ Ipc::ClearListenFor(Ipc::mtCacheMgrRequest);
+ Ipc::ClearListenFor(Ipc::mtCacheMgrResponse);
+}
+bool
+CacheManager::doneAll() const
+{
+ // Once started, only exit on shutdown.
+ return shutting_down != 0;
+}
Please call the parent's method at the end of the above methods (even
though the current parent's method may not do anything).
+ Subscription::Pointer sub;
+ RefCount<IpcMsgCall> call;
It is better to avoid creating Pointer-like variables until they are
needed as it saves CPU cycles and simplifies code.
Also, If you can make them const, please do so.
+ debugs(16, 1, "Startup: CacheManager: Accepting manager IPC
responses");
+ debugs(16, 1, "Startup: CacheManager: Accepting manager IPC requests");
Use DBG_IMPORTANT if you think these debugging messages warrant level-1
reporting. IMO, they should use level 2.
+ // XXX: Just run the action? we are inside an async manager call already.
+ Mgr::Action::Pointer action = createRequestedAction(request.params);
+ if (Ipc::Coordinator::Instance())
+ AsyncJob::Start(new Mgr::Inquirer(action, request,
Ipc::Coordinator::Instance()->strands()));
+ // else throw? coordinator scheduled a call and abandoned us.
This feels very awkward or wrong, but perhaps I am just missing something:
Probably me who is missing something (or a lot). This is my first
attempt to make SMP messages work after all :)
1) Running the action in Coordinator requires a lot of messages. So the
XXX comment cannot apply to the in-Coordinator code. That code does need
a Mgr::Inquirer job!
K. XXX was to get your attention and it worked :). Will drop it now.
That code is in CacheManager::, at this point I'm not entirely sure what
actualy runs where.
I am assuming that the request was scheduled by an Ipc::Coordinator
object in the current process. Otherwise what else would SMP use to
schedule Mgr::Request calls for CacheManager to pick up?
2) If Ipc::Coordinator::Instance() is nil, I assume the code is running
inside a regular kid. Regular kids should not even know about
Coordinator class and all the stuff that comes with it. The only thing
they should know about Coordinator is how to send it an IPC message. It
is wrong, IMO, for the CacheManager class code to depend on Coordinator
class.
It has assumed the code snippet from src/ipc/Coordinator for handling of
a Mgr::Request. I am assuming that if Coordinator exists in the current
process to schedule *this call. If not, what did? I need to merge that
codes reply bits into here too.
3) The "else throw?" comment looks strange because regular kids do not
instantiate a Coordinator object (I hope!). They should actually perform
the request. I do not see the code that actually runs the action in the
new CacheManager::handleIpcRequest(), which is exactly the place I would
expect that code to be.
Aha! One more likely cause of my hung response problem.
Overall, there seems to be some confusion (possibly on my part!) about
what kind of process/class would run the above code. IIRC, it should be
Coordinator process/class, not the general kid process and the general
cache manager class. The cache manager class should have the code to
actually run the action locally, at Coordinator request.
I will stop reviewing here as it will probably be a waste of time
whether I am confused or the code.
Okay. You have brought up a few things I need to ask about before going
further.
I was under the dubious assumption that each SMP process had an
Ipc::Coordinator _Job_ which read UDS packets and called the appropriate
function based on type. With the coordinator _process_ version of that
Job doing more shovelling around stuff in its actions than the workers.
- my change #1 was to make that function an AsyncCall and schedule it
instead of calling synchronously. All fine. Nothing that could cross
processes there.
- my questionable change (#2) was to make the CacheManager a Job which
was instantiated during reconfigure/startup. (that would be one for each
worker yes?)
- during setup that Manager Job uses Ipc::Coordinator::Instance()
and registers its Mgr::Request/Mgr::Reply actions AsyncCalls to receive
mgr: packets. (that might start in Ipc::Coordinator in each worker?)
What I am trying to achieve is:
- worker A received mgr:action URL. Sends Mgr::Request (UDS packet)
to ?1?.
- ?2? schedules Manager:: subscription call to handle the
packet/TypedMsgHdr
- Question: manager ACKs receipt with Mgr::Response immediately?
- CacheManager handling it schedules UDS requests for all the workers
through ?2?
- ?3? in the worker B-F receives UDS packet and produces a reply, and
passes it back to ?3?
- ?2? passes the worker B-F replies to manager which merged and send
the reply via UDS to worker A.
Halfway sane? or batting off left field?
?1? and ?2? would be the same Ipc::Coordinator Job right? or is ?1? a
worker UDS handler and ?2? the coordinator process UDS handler?
(gah, looks like a mess, hope you can read that.)
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.15
Beta testers wanted for 3.2.0.10
