Hi,

Yes -- I think in general, the meaning of "status" with regards to a connection processing (or a filter that may preempt it) was that "a valid HTTP response was sent..." If you look deep in the rat's nest of connio.c, you'll find:

int
Ns_WriteConn(Ns_Conn *conn, char *buf, int len)
{
    if (Ns_ConnWrite(conn, buf, len) != len) {
        return NS_ERROR;
    }
    return NS_OK;
}


Ns_ConnWrite eventually calls the "send" routine from the driver (sock or ssl). If the driver can't send the # of bytes intended, it returns NS_ERROR. This turns any network send error into a simple "couldn't send" error. That code was the original place where NS_ERROR could be generated and made sense when it was sending content from a file.

There are some complications -- originally the headers were sent before the content, i.e., there was no buffering. That was a performance irritant so a level of buffering was put in place and so results which previously may have meant "headers not sent due to dropped connection" now silently buffer result and return NS_OK always. Then with ADP there grew some encoding issues and/or gzip'ing goals that also led to more of "get a big whole buffer ready and blast that in one attempt, using iovec's if possible..." You can see that in the Ns_ConnFlush and Ns_ConnFlushDirect complexity.

So, the simple "NS_ERROR means conn failed..." isn't so valid anymore. In fact, on a long running connection the code could cheerfully be creating a buffer to send over a socket that may have already dropped. That knowledge -- async socket drop -- is actually something the I/O event thread (sometimes confusing called the driver thread for the DriverThread routine in driver.c) could easily detect. Event-driven socket code is often more efficient than thread-based I/ O. For AOLserver, the read-side was made event-driven years ago (helps with denial of service attacks and general efficient) but the write side remained thread-based mostly because of the expectations (smart or dumb) of traces and order of execution, i.e., you have a thread that gets the read-ahead buffer, does the work, sends (synchronously) the response, and the proceeds to the traces, cleanup, etc. Note to add some confusion, the I/O thread currently gracefully closes connections already -- it's possible that generates an error condition which is never accurately logged (note the comment "NB: Timeout Sock on end-of-file or error." -- result becomes "pretend it actually worked and just close socket...).


If I had to do this all over again, I'd have all the I/O be event driven. It would read the request as it does now, allow for some async-work if needed (possible now but not used as far as I know), dispatch the complete request context to an execution thread to do script-stuff and provide result back to the I/O thread where event- driven code would send result and generate a smart log entry, including all cases of request bad, code failed, send failed, or all ok. If while the thread was executing the I/O thread detected a dropped socket, it could send an async-exception message to the execution thread which may or may not be noticed or handled but an attempt could be made.

While this may be nifty, today it would mean traces and filters would run before the content was already sent. For a connection cleanup (i.e., dump this temp resource), it would make sense but for an old- school common log format it may be odd (or at least suspect, but perhaps no less suspect than what's going on now).


-Jim




On Apr 13, 2009, at 2:48 PM, Tom Jackson wrote:

On Mon, 2009-04-13 at 13:49 -0400, Jim Davidson wrote:
Hi,

A bit old but let me try to be helpful here...


On Apr 3, 2009, at 11:45 PM, Tom Jackson wrote:

....

Jim,

Thanks for adding some back story. Over the weekend I spent more time
looking into this problem and basically rewrote one portion of ConnRun
and pulled out the auth part of the code into another static function.

As Gustaf noticed, there was a little confusion in the code, I think
mostly related to the use and meaning of the "status" variable. This
variable has several meanings at different points in ConnRun and it gets confusing. Plus I wasn't completely sure what was supposed to happen at
each point.

I think I finally figured it out, and it now appears to me that
including the access logging trace where it is makes perfect sense. What
didn't make sense, and was a bug, was the meaning of the value of
"status" at the point trace filters, server traces and void traces ran.

In effect, "status" must indicate the success/failure of sending a
response to the client. The current code didn't do that. Here is my
rewrite of the two affected functions, with comments added:

/*
*----------------------------------------------------------------------
*
* ConnAuthorize --
*
*      Try to authorize a connection.
*
* Results:
*      NS_OK on successful authorization,
*      NS_FILTER_RETURN on authorization failure, or
*      NS_ERROR on error.
*
* Side effects:
*      Connection request is authorized. On failure an alternative
*      response may be sent to the client.
*
*----------------------------------------------------------------------
*/

static int
ConnAuthorize(Conn *connPtr)
{
   Ns_Conn        *conn = (Ns_Conn *) connPtr;
   NsServer       *servPtr = connPtr->servPtr;
   int            status;

   status = Ns_AuthorizeRequest(servPtr->server,
                connPtr->request->method, connPtr->request->url,
                connPtr->authUser, connPtr->authPasswd, connPtr->peer);

   switch (status) {
   case NS_OK:
       break;
   case NS_FORBIDDEN:
       /*
        * NS_OK indicates that a response was sent to the client
        * a this point, we must fast-forward to traces,
        * so we convert the NS_OK to NS_FILTER_RETURN.
        */
       if ((status = Ns_ConnReturnForbidden(conn)) == NS_OK) {
            status = NS_FILTER_RETURN;
       }
       break;
   case NS_UNAUTHORIZED:
       /*
        * NS_OK indicates that a response was sent to the client
        * a this point, we must fast-forward to traces,
        * so we convert the NS_OK to NS_FILTER_RETURN.
        */
       if ((status = Ns_ConnReturnUnauthorized(conn)) == NS_OK) {
           status = NS_FILTER_RETURN;
       }
       break;
   case NS_ERROR:
   default:
       status = NS_ERROR;
       break;
   }

   return status;
}


/*
*----------------------------------------------------------------------
*
* ConnRun --
*
*       Run a valid connection.
*
* Results:
*       None.
*
* Side effects:
*       Connection request is read and parsed and the cooresponding
*       service routine is called.
*
*----------------------------------------------------------------------
*/

static void
ConnRun(Conn *connPtr)
{
   Ns_Conn        *conn = (Ns_Conn *) connPtr;
   NsServer       *servPtr = connPtr->servPtr;
   int            i, status;
   Tcl_Encoding    encoding = NULL;
        
   /*
    * Initialize the connection encodings and headers.
    */

   connPtr->outputheaders = Ns_SetCreate(NULL);
   encoding = NsGetInputEncoding(connPtr);
   if (encoding == NULL) {
        encoding = NsGetOutputEncoding(connPtr);
        if (encoding == NULL) {
            encoding = connPtr->servPtr->urlEncoding;
        }
   }
   Ns_ConnSetUrlEncoding(conn, encoding);
   if (servPtr->opts.hdrcase != Preserve) {
        for (i = 0; i < Ns_SetSize(connPtr->headers); ++i) {
            if (servPtr->opts.hdrcase == ToLower) {
                Ns_StrToLower(Ns_SetKey(connPtr->headers, i));
            } else {
                Ns_StrToUpper(Ns_SetKey(connPtr->headers, i));
            }
        }
   }

   /*
    * Run the request.
    */

   while (1) {

       /*
        * Proxy requests replace request logic
        */

       if (connPtr->request->protocol != NULL
           && connPtr->request->host != NULL) {

           status = NsConnRunProxyRequest((Ns_Conn *) connPtr);
           break;
       }

       /*
        * Each stage of request processing can return one of three
        * possible results:
        * NS_OK means continue to next stage
        * NS_FILTER_RETURN means skip to traces
        * NS_ERROR means skip to 500 response attempt
        */

       status = NsRunFilters(conn, NS_FILTER_PRE_AUTH);
       if (status != NS_OK) {
           break;
       }

       status = ConnAuthorize(connPtr);
       if (status != NS_OK) {
           break;
       }

       status = NsRunFilters(conn, NS_FILTER_POST_AUTH);
       if (status != NS_OK) {
           break;
       }

       status = Ns_ConnRunRequest(conn);
       break;
   }

   if (status == NS_ERROR) {
       status = Ns_ConnReturnInternalError(conn);
   }

   /*
    * Closing connection also runs code registered with
    * ns_atclose.
    */

   Ns_ConnClose(conn);

   /*
    * Trace filters, void filter traces and server traces
    * run a response was sent to the client
    * Note that a successful response includes sending
    * an internal server error response.
    */

   if (status == NS_OK || status == NS_FILTER_RETURN) {
      /*
       * Filter Traces are registered with ns_register_filter
       * Ignore the return code? We need to do NsRunTraces based
       * upon status before trace filters ran. Question is if we
       * should skip void trace filters. We could wrap them in another
       * status check, then move on to NsRunTraces.
       */
       NsRunFilters(conn, NS_FILTER_TRACE);
      /*
       * Filters registered outside the context of a particular server
       */
       (void) NsRunFilters(conn, NS_FILTER_VOID_TRACE);
      /*
       * Server traces registered with Ns_RegisterServerTrace
       * Access logging is handled at this point.
       */
       NsRunTraces(conn);
   }

   /*
    * Cleanup the connections, calling any registered cleanup traces
    * followed by free the connection interp if it was used.
    */

  /*
   * Cleanups are registered with either:
   * Ns_RegisterConnCleanup: same signature as Ns_RegisterServerTrace
   * Ns_RegisterCleanup: runs for all servers
   */

   NsRunCleanups(conn);

   /* Finished with conn */

   NsFreeConnInterp(connPtr);
   if (connPtr->type != NULL) {
       Ns_ConnSetType(conn, NULL);
   }
   if (connPtr->query != NULL) {
       Ns_ConnClearQuery(conn);
   }
   if (connPtr->outputheaders != NULL) {
        Ns_SetFree(connPtr->outputheaders);
        connPtr->outputheaders = NULL;
   }
   Ns_DStringTrunc(&connPtr->obuf, 0);
}


tom jackson


--
AOLserver - http://www.aolserver.com/

To Remove yourself from this list, simply send an email to <lists...@listserv.aol.com > with the body of "SIGNOFF AOLSERVER" in the email message. You can leave the Subject: field of your email blank.


--
AOLserver - http://www.aolserver.com/

To Remove yourself from this list, simply send an email to 
<lists...@listserv.aol.com> with the
body of "SIGNOFF AOLSERVER" in the email message. You can leave the Subject: 
field of your email blank.

Reply via email to