This is what I suggested a few emails ago :-)
> 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.
I'm fine with this patch.
-Andrew
On Fri, Apr 3, 2009 at 2:37 PM, Tom Jackson <t...@rmadilo.com> wrote:
> 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 <t...@rmadilo.com> 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
> > > <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.
> > >
> > >
> > >
> > > --
> > > 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.
> >
>
>
> --
> 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.
