Re: [naviserver-devel] nsproxy API

[Zoran V. <zv...@ar...>]

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