Hey,
Hopefully this is my last post on this subject, I think I actually found
the bug.
The bug is in NsAdpFlush from nsd/adprequest.c:
214- */
215-
216- Tcl_ResetResult(interp);
217: if (itPtr->adp.exception == ADP_ABORT) {
218- Tcl_SetResult(interp, "adp flush disabled: adp aborted",
TCL_STATIC);
219- result = TCL_OK;
220- } else if (len == 0 && stream) {
The bug was a missing line setting result to TCL_OK. (line 219).
Also, ns_adp_return cannot be used after and ns_returnxxx command as adp
processing continues after calling it.
Here are two test files:
test-adp-abort.adp:
<%
ns_return 200 text/plain hi
ns_adp_abort
%>
test-adp-return.adp:
<%
ns_adp_puts hi
ns_adp_return
%>
Both of these result in an access.log entry.
Before the change, ns_adp_abort would lead to an error message:
adp flush failed: connection closed
abort exception raised
while processing connection #2:
GET /test.adp HTTP/1.1
Host: 127.0.0.1:8000
User-Agent: ...
Accept: ....
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive
Cache-Control: max-age=0
This error message is valid if ns_adp_return is used after and
ns_returnxxx.
tom jackson
On Fri, 2009-04-03 at 08:33 -0700, Tom Jackson wrote:
> Andrew,
>
> I wasted a little more time looking at the actual code. My impression is
> that everything is working as expected. If there is an error in a
> postauth filter or in adp processing (registered proc), trace filters
> are skipped. Until about 4.5, errors during preauth also skipped trace
> filters. Not sure why this change was made.
>
> The only think that matters is what happens in Ns_AdpRequest. If there
> were no errors, the request will be logged. In order to get ns_adp_abort
> to work correctly, the tcl result must be set to TCL_ERROR until code
> returns to Ns_AdpRequest. This is why an additional structure is
> maintained for the adp exception, which is independent of the tcl
> exception. In this case, adp.exception indicates what actually happened
> during adp processing.
>
> So things seem to be working as intended, and they have been working the
> same way for a long time. It might be possible that you are misusing
> ns_adp_abort, or something else is messing up.
>
> Could you provide a simple test case, probably a few nested adp
> includes, which repeats the issue? Without a test case of what you think
> should work differently, it is hard to give any more advice.
>
> In general, when an error occurs during a request, the response is by
> definition an error response, so the original request might get
> transformed into an internal redirect to your error handling page. An
> error in this page, or a missing error page could cause further
> problems.
>
> Bottom line: no reason to believe that this is a bug.
>
> tom jackson
>
> p.s. this case seems to validate my belief that the hardest bug to find
> and fix is one that doesn't actually exist.
>
> On Thu, 2009-04-02 at 18:03 -0500, Andrew Steets wrote:
> > What was the original purpose of "trace" filters? At the C API level
> > there is a distinction between between a trace filter and a cleanup
> > callback, but it doesn't look like you can register a cleanup proc
> > from TCL. Maybe this was mistakenly omitted?
> >
> > The cleanup procs run unconditionally. It seems like that is the most
> > appropriate place to handle "cleanup of resources." Alternatively we
> > could change the trace filters to run regardless of the
> > Ns_ConnRunRequest() return status, but then that would make them
> > basically the same as the cleanups.
> >
> > I looked a little deeper into the source. The confusion seems to
> > arise in NsAdpFlush() which is run at the end of all ADP processing.
> > The code there is smart enough to recognize when an abort exception
> > has been signalled; it sets the TCL result to "adp flush disabled: adp
> > aborted", but it still returns TCL_ERROR. That is essentially where
> > the TCL exception gets turned into a full blown connection processing
> > error. We could change NsAdpFlush() to return success when it
> > recognizes the abort exception, or just not run NsAdpFlush() for abort
> > exceptions.
> >
> > There would still be cases where trace filters would not run though.
> > For instance if you called ns_returnxxx without calling ns_adp_abort.
> > I'm not sure if that is a bad thing.
> >
> > It would be nice to hear from anyone who knows about the original
> > motivation for the trace and cleanup filters.
> >
> > -Andrew
> >
> > On Thu, Apr 2, 2009 at 3:53 PM, Tom Jackson <[email protected]> wrote:
> > Gustaf,
> >
> > You may be "using" traces but not realize it, it sounds like
> > ns_adp_abort isn't don't what was originally intended.
> >
> > I wouldn't worry about an runtime error caused during running
> > traces, it
> > would be an error to even use ns_adp_abort in a trace filter
> > because the
> > connection is already finished. This is analogous to calling
> > [break]
> > outside of a loop.
> >
> > It seems important to consider ns_adp_abort, ns_adp_return and
> > ns_adp_break as a unit. They add necessary loop type controls
> > so that
> > developers can create deeply nested code and still get out of
> > it without
> > the need to use [catch]. But, like a lot of AOLserver specific
> > procedures, there is no hand-holding in their use. They can be
> > misued.
> >
> > In this particular case, it looks like somewhere along the
> > way,
> > ns_adp_abort was modified to not work as expected.
> >
> > The desired effect is exactly what you would get by returning
> > filter_return from a preauth or postauth filter. This effect
> > is to skip
> > to trace filters, not past them.
> >
> > Skipping trace filters even on an aborted connection would be
> > a disaster
> > for any application which relies on cleanup of resources.
> >
> > tom jackson
> >
> >
> > On Thu, 2009-04-02 at 11:12 +0200, Gustaf Neumann wrote:
> > > Andrew Steets schrieb:
> > > > The patch I sent earlier seems to fulfill these needs, but
> > I am
> > > > worried about corner cases where LogTrace (from the nslog
> > module)
> > > > could blow up. Nothing about the state of the Conn *
> > seems to be
> > > > guaranteed when the ConnCleanup callbacks are called.
> > > >
> > > Dear Andrew,
> > >
> > > i think most (all?) of the repondents seems to agree that
> > writing in the
> > > about case to
> > > the access log file. For me there are still two quesions
> > open:
> > >
> > > a) is it possoble to call ns_adp_abort at some time, where
> > the server
> > > might crash
> > > (in normal operations, everthing looks fine to me,
> > problems might
> > > occur in
> > > when called from some traces; other calls are likely to
> > have similar
> > > problems)
> > >
> > > b) the patch replaces the call to the regular server trace
> > by a
> > > connection cleanup call.
> > > this means, at least in 4.5.*, ns_adp_abort seems to
> > cancel all
> > > traces (also
> > > these registered with ns_register_trace). Is this
> > desired?
> > >
> > > From Tom's website:
> > http://rmadilo.com/files/nsapi/ns_adp_abort.html
> > > the doc of ns_adp_abort says
> > >
> > > ... Every ns_returnxxx call in an ADP should be followed
> > with a call
> > > to ns_adp_abort....
> > >
> > > With this recommendation, cancelling traces seem wrong to
> > me; or at
> > > least,
> > > this should be documented.
> > >
> > > We don't use traces, all of OpenACS does not use it, so this
> > is no
> > > current issue for us.
> > >
> > > -gustaf neumann
> > >
> > >
> > > --
> > > AOLserver - http://www.aolserver.com/
> > >
> > > To Remove yourself from this list, simply send an email to
> > <[email protected]> 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
> > <[email protected]> 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
> > <[email protected]> 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
> <[email protected]> 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
<[email protected]> with the
body of "SIGNOFF AOLSERVER" in the email message. You can leave the Subject:
field of your email blank.
