Bug 61164 - Add %X option to access log for connection status
Summary: Add %X option to access log for connection status
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Catalina (show other bugs)
Version: unspecified
Hardware: PC Linux
: P2 enhancement (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-07 08:52 UTC by Mark Thomas
Modified: 2017-07-28 14:54 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Thomas 2017-06-07 08:52:12 UTC
In Apache HTTPD there is an option in their access log format to log
connection status: "%X"

http://httpd.apache.org/docs/2.4/mod/mod_log_config.html#formats

"%X":
Connection status when response is completed:
X = Connection aborted before the response completed.
+ = Connection may be kept alive after the response is sent.
- = Connection will be closed after the response is sent.
Comment 1 Zemian Deng 2017-07-21 03:17:28 UTC
Hi there,

I have created a PR here (https://github.com/apache/tomcat/pull/70) for this enhancement. Let me know what you think.
Comment 2 Mark Thomas 2017-07-24 16:16:50 UTC
This is a lot simpler than I imagined. My expectation was that the various places where errors can occur in  in Http11Processor.service() would make this tricky to implement. It looks like I was wrong. Because those errors set the RequestDispatcher.ERROR_EXCEPTION, this actually makes implementation fairly simple.

One aspect that I think needs a little more thought is container initiated (rather than client initiated) aborts. The easiest way to detect these would be to add another request attribute. Whether both get logged as 'X' or whether one is logged as 'X' and the other as 'x' is open to discussion.
Comment 3 Zemian Deng 2017-07-24 20:27:57 UTC
Hi Mark, yes hence the PR wasn't that big. :)

I saw the comment you made on GitHub, I will take a further look later and let you know.
Comment 4 Zemian Deng 2017-07-25 02:45:36 UTC
I have pushed a new commit to the PR. Please review.

As far as adding "container initiated (rather than client initiated) aborts", I will need to study some more before I can determine where to add the new req attr. (do give me a hint if you already know.)
Comment 5 Zemian Deng 2017-07-25 03:26:54 UTC
Mark, I see org.apache.coyote.AbstractProcessor#dispatch() starting line 205 contains error handling code that says "occurred on 'non-container' thread". Is this where what you want to start tracking with a new req attribute?
Comment 6 Mark Thomas 2017-07-25 07:59:28 UTC
The updated patch looks good. Thanks. I noticed that the import order has changed. If you could undo that it would be good but it isn't a big deal to fix when the patch is applied.

Regarding tracking container vs client connection abort, I was looking at AbstractProcessor#setErrorState() around line 90. Having thought about this some more, a request attribute is a bit of a hack and I think I can see a batter way. 

What I was thinking was to also log 'X' if AbstractProcessor#getErrorState()#isIoAllowed() returns false. The correct way to access the Processor via the Request or Response is by defining an ActionCode and calling Request#acton() or Response#action(). Take a look at ActionCode#IS_ERROR and how it is used. I am thinking ActionCode#IS_IO_ALLOWED and then call Request#action() from the access log.

I'm not sure if there is value in differentiating client and container aborts. I'm leaning towards not differentiating for now. If we add support for '%X' now that does not differentiate, we can always add '%x' at a later date that does differentiate if there is demand.
Comment 7 Zemian Deng 2017-07-26 02:46:16 UTC
Mark, thanks for all the tips. No problem, I can re-fix the imports for better merge experience.

Yes, I agree that we should support just '%X' for now until users demands the finer abort types, then we can add the extra '%x' pattern.

As for the extra '%X' check with ErrorState()#isIoAllowed() == false, I assume you wanted for this patch. I can give it a try and see. Will update later.
Comment 8 Zemian Deng 2017-07-26 03:47:40 UTC
I have added the isIoAllowed with action code check to the PR now. Please review.
Comment 9 Mark Thomas 2017-07-28 14:54:06 UTC
Thanks for the patch. It has been applied to trunk (for 9.0.0.M26 onwards) and 8.5.x (for 8.5.20 onwards)