My original concern was with the access logging proc, which happens to be run as a trace filter. I think that the access log entries should be generated regardless of whether or not ns_adp_abort is called. I don't care too much about anything else that was installed as a trace filter.
Do you agree that access log entries should be generated if ns_adp_abort is called? -Andrew On Fri, Apr 3, 2009 at 10:33 AM, Tom Jackson <[email protected]> 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.
