bugGNUstep - Bugs: bug #33181, (WebServer 1.4.0) WebSerber`s...

Group
 
 

bug #33181: (WebServer 1.4.0) WebSerber`s -_audit: is called 3 times per 1 request

Submitter:  T.Somequare <tsomeqdev>
Submitted:  Fri 29 Apr 2011 05:42:37 PM UTC
   
 
Category:  Libraries Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  In Test
* Mandatory Fields

Add a New Comment Rich Markup
   

Thu 05 May 2011 04:14:11 PM UTC, comment #5: 

My long-term aim is to make the WebServerConnection class public.
If/when that's done, it would be possible to change the content of the logged audit string by overriding the -audit method ... in a subclass of WebSerbverConnection.
Doing that would be efficient and simple.

Richard Frith-Macdonald <CaS>
Group Member
Thu 05 May 2011 02:18:29 PM UTC, comment #4: 


> Only if we added new API for this (or broke all existing code using the library) ..

All right.

 
Currently the WebServerConnection class and WebServer`s -_audit method are private.
So the behavior could not be changed without parsing and replacing passed string or modifying library.

How about to split implementaion like below?

//---
WebServerConnection.m
- -(NSString*)audit
+ -(id)audit
{
   NSDate *d;
   NSString *h;
   /* ... ( code set local variables is here) ... */
+   NSDictionary* info = [[NSDictionary alloc]initWithObjectsAndKeys:h,@"host"
+    ,u,@"user", d , @"date", c, @"command", a, @"agent", r, @"result", nil];
+  id audit = [server provideAuditFor:info];
+  [info release];
+  return audit;
-   return [NSStringClass stringWithFormat: @"%@ - %@ [%@] %@ %@ %@",h,u,d,c,a,r];
}

WebServer.m
+ -(id)provideAuditFor:(NSDictionary*)info
+ {
+    NSString* h = [info objectForKey:@"host"];
+    /* ... ( code to get other parts and put them into local vars) ... */
+    return [NSString stringWithFormat:@"%@ - %@ [%@] %@ %@ %@",h,u,d,c,a,r];
+}
//---

This will give a chance to create custom message with writing a subclass of WebServer and override the method +provideAuditFor: .
Although this is a bit expensive and complex way, considering additional information (cookie, env, other headers, etc) would be supported in future.

This is just an opinion . Please show us if you have any better idea.

Thanks anyway.



T.Somequare <tsomeqdev>
Thu 05 May 2011 10:15:57 AM UTC, comment #3: 


> I thought this change would enable the delegate to log string in its own format.


Only if we added new API for this (or broke all existing code using the library) ... I don't think it's enough of an advantage unless a lot of people want to do it.

> This is a loss of performance, isn`t it ?

Technically yes, but in practice it's so tiny as to be unimportant.  However, I added caching of the results of the test for the audit method (since it's always called at least once per request).  However, it's arguable that none of the four tested delegate methods should be cached this way ... any performance gain is tiny and code which explicitly calls -respondsToSelector: immediately before using the selector might be clearer.

Richard Frith-Macdonald <CaS>
Group Member
Thu 05 May 2011 01:23:30 AM UTC, comment #2: 

Thank you for the fix.


>  I don't understand what the advantage would be of having
> the -audit method return an instance of a class containing
> the audit information rather than the string to be logged


I thought this change would enable the delegate to log string in its own format.
It also enables to suppress too long output , because some client often sends very long header as user-agent.
It`s nice for users convenience if  the -description method of it returns default formatted string like currently returned by -audit.

By the way, I found the WebServer`s -setDelegate: method tests whether the delegate responds to several selectors,
but -webAlert:for:, -webLog:for:, -webAudit:for: are not tested.
This is a loss of performance, isn`t it ?

T.Somequare <tsomeqdev>
Wed 04 May 2011 11:37:04 AM UTC, comment #1: 

Thanks ...

There is supposed to be a line with no request details logged when the connection is dropped, so it's actually one extra line (I fixed in svn trunk).

However, for consistency with apache I also modified the code to refrain from logging the connection end in cases where at least one request had been received on the connection and verbose logging is not enabled.

So behavior now should be:
If verbose logging is on, then one line for each request and one line for connection end.
If verbose logging is off, then one line for each request and one line for a connection ended without any request.

I don't understand what the advantage would be of having the -audit method return an instance of a class containing the audit information rather than the string to be logged

Richard Frith-Macdonald <CaS>
Group Member
Fri 29 Apr 2011 05:42:37 PM UTC, original submission:  

I think The WebServer class has a delegate method "webAudit:for:" for
logging a distinct access like the  Apache`s access_log.
But this is called 3 times per 1 access.
This method seems, there are multiple referrers listed bellow:
  WebServer -(void)_endConnect:(WebServerConnection*)connection
  WebServerConnection - (void) _didWrite: (NSNotification*)notification
  WebServerConnection - (void) end

This causes 2 useless lines. they offten print incomplete string like "- - -".
And calling respondsToSelector: @selector(webAudit:for:) are also .

I have an idea;
 Currently the WebServerConnection`s -audit returns an instance of NSString.
This NSString* consists of informations like timestamp, address, user-agent, and so on.
 I think it is better to return an object of class newly created to hold logging record.
 And it implements methods indicates each information...
@interface WebServerLogRecord
-(NSString*)address;
-(NString*)agent;
-(NSDate*)date;
...
Then, additionally the "-phase" method to return a state end reading/connect/ end writing.
-(WebServerPhase)phase;

What about this?


T.Somequare <tsomeqdev>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by CaS (Posted a comment)
  • -email is unavailable- added by tsomeqdev (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-05-04 CaS StatusNone Fixed
        Open/ClosedOpen In Test

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code