With the additional patch for the start times and connect times and the
TimeMonitor, I feel this is in good enough shape to be committed, so I
checked it in. I also wrote some additional tests for this.
I did find a small anomaly with my tests, however. I was looking for a
case that would trigger the CONNECTION_CLOSED event, and discovered
there are no places in the code where that event is raised. It appears,
in general, that close events are handled by the server closing the
connection and these come through as CONNECTION_CLOSED_BY_SERVER
events. Ok, perhaps the CONNECTION_CLOSED event is not needed.
However, when writing some tests for the redirection cases, it appears
that when we receive a redirection response back from the server, we're
not getting a CONNECTION_CLOSED_BY_SERVER event. If the connection is
not cached, we appear to ended up with a dangling live connection.
Should something to be done to explicitly close this connection in that
situation?
Rick
Sangjin Lee wrote:
On Jan 23, 2008 6:50 AM, Rick McGuire <[EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]>> wrote:
Only if it can be done without having to maintain the same sort of
request-to-start time map that you don't wish to do with the listener.
The process of adding data collection should cause memory bloat there
either, particularly if monitoring is not being used (the more likely
case). It seems more reasonable that this type of processing
should be
pushed into the monitoring implementation rather than have the async
client try to keep track of everything. This way, the overhead is
only
introduced while metrics are being gathered. A very simple and
relatively low cost means might be to add a couple of time stamp
fields
to the request object, but only for the most significant events.
Perhaps request start and connection start, but nothing else. Another
possible approach would be to have a mechanism that would allow the
monitor to attach an annotation object to the request that could
be used
to implement a lifecycle memory if needed. The cost of doing this is
relatively minor when this type of information is not needed, but it's
flexible enough to be tailored to any type of data collection.
I agree, the only things that make sense to be present are request
start time and connect start time... I'm going to upload a revised
patch that shows this along with a proof-of-concept monitor that uses
this to compute the average response time.
Thanks,
Sangjin
