[Bug 61164] Add %X option to access log for connection status

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=61164

Mark Thomas  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--- Comment #9 from Mark Thomas  ---
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)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61164] Add %X option to access log for connection status

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=61164

--- Comment #8 from Zemian Deng  ---
I have added the isIoAllowed with action code check to the PR now. Please
review.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61164] Add %X option to access log for connection status

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=61164

--- Comment #7 from Zemian Deng  ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61164] Add %X option to access log for connection status

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=61164

--- Comment #6 from Mark Thomas  ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61164] Add %X option to access log for connection status

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=61164

--- Comment #5 from Zemian Deng  ---
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61164] Add %X option to access log for connection status

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=61164

--- Comment #4 from Zemian Deng  ---
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.)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61164] Add %X option to access log for connection status

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=61164

--- Comment #3 from Zemian Deng  ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61164] Add %X option to access log for connection status

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=61164

--- Comment #2 from Mark Thomas  ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61164] Add %X option to access log for connection status

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=61164

--- Comment #1 from Zemian Deng  ---
Hi there,

I have created a PR here (https://github.com/apache/tomcat/pull/70) for this
enhancement. Let me know what you think.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org