On 18.09.2006, at 20:55, Stephen Deasey wrote:
Naming is easy -- it's the explicit handle management that's making
things different. Low level C APIs just don't map directly into C.
For example, you never expect a C programmer to grab a handle then
leave it dangling, that's just broken code. But with Tcl this sort of
thing is expected. You have to clean up, take extra precautions with
locking, and so on.
As far as I can see, the handles aren't needed. How about
something like this:
API:
result ns_exec_eval ?-pool p? ?-timeout t? script
Id ns_exec_queue ?-detached? ?-pool p? ?-timeout t? script
ns_exec_wait ?-timeout t? Id ?Id ...?
ns_exec_cancel
ns_exec_set ?-opt val ...? pool
ns_exec_stats pool
Example:
set result [ns_exec_eval {
set x y
exec /bin/blah $x
}]
Notes:
Create pool 'default' on startup.
Reuse identical processes (same binary, ulimits, etc.) among
pools, in
the same way that threads are shared globally among ns_job
queues.
'maxexecs' parameter, after which processes exit and are
respawned.
As well as not being very descriptive, the name 'proxy' also clashes
with genuine HTTP proxy functions such as ns_register_proxy. The
proxy functionality that Vlad's been adding recently obviously should
be in a a module called nsproxy...
Some things are definitely version 2, ulimits for example. It would
be a pain to get stuck with an awkward API though.
What do you think?
All kinds of good ideas... I will comment on that tomorrow
when I'm back to work.
I noticed a couple of things while looking through the code:
nsproxymod.c: SrvMod structure is statically allocated, one per
process, but is assigned to each time the module is loaded,
potentially x times per virtual server.
Yes. Incorrect. Must fix that. There is no harm except
memory leaking... but this is bad enough.
nsproxylib.c, ProxyPop(): Extra call to Ns_CondBroadcast() without
lock being held.
if (err != ENone) {
while ((proxyPtr = firstPtr) != NULL) {
firstPtr = proxyPtr->nextPtr;
PushProxy(proxyPtr);
}
Ns_CondBroadcast(&poolPtr->cond);
return TCL_ERROR;
}
?? You need not necessarily hold a locked mutex to broadcast the
condition
varible. In this case the PushProxy is making necessary locks.
The Ns_CB is called here to release waiters at line 2067 or 2070
after we have retuned pop'ed proxies back to the pool in case of error.
nsproxylib.c, ProxyPush(): Ns_CondBroadcast() called whether proxy is
returned to pool or not.
OK. This is true. Actually, we only need this one. The other Ns_CB
(above)
can be removed. Will make necessary modifications.
Ns_MutexLock(&poolPtr->lock);
poolPtr->nused--;
poolPtr->nfree++;
if ((poolPtr->nused + poolPtr->nfree) <= poolPtr->max) {
proxyPtr->nextPtr = poolPtr->firstPtr;
poolPtr->firstPtr = proxyPtr;
if (proxyPtr->procPtr) {
SetExpire(proxyPtr->procPtr, proxyPtr->timers.tidle);
}
proxyPtr->timers = poolPtr->timers;
proxyPtr = NULL;
} else {
poolPtr->nfree--;
}
Ns_CondBroadcast(&poolPtr->cond);
Ns_MutexUnlock(&poolPtr->lock);
GetPool(): Proxy pool is created if name pool name does not already
exist. What about typos? This is error prone. Alternative: always
create a pool 'default' on start up and make specifying pool names
optional?
This was the original code I just took as-is and made "usable" and
working. Of yourse, if you change this then all other options are
open.... But more on that tomorrow.
The term 'procPtr' is confusing when used to mean pointer to Proxy
Slave. In the rest of the code base, procPtr means pointer to C
function, i.e. a callback.
How about slavePtr?
Cheers
Zoran
