Fixing behavior of logback-access in the modern era #532
Conversation
+ Fixes LOGBACK-1580 to report the proper
HttpServletResponse.getStatus() that was used
when the response was committed.
+ Works around LOGBACK-1581 in AccessEvent to
not fail logging when the
HttpServletRequest.getParameterNames() or
HttpServletRequest.getParameter(String)
methods are used against unread or even bad
request objects.
+ Updated Javadoc (needs to be copied over
to logback manual though)
+ Must use OpenJDK 1.8 to compile and release.
+ Still compiles to Java 1.6 bytecode for release.
Signed-off-by: Joakim Erdfelt <[email protected]>
joakime
force-pushed
the
fix-jetty-requestlog
branch
from
b18f8b3
to
bf66c83
Compare
logback-access/src/main/java/ch/qos/logback/access/jetty/RequestLogImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Joakim Erdfelt <[email protected]>
| String key = e.nextElement(); | ||
| requestParameterMap.put(key, httpRequest.getParameterValues(key)); | ||
| } | ||
| } catch(Throwable t) { |
There was a problem hiding this comment.
The Servlet Spec doesn't specify the exceptions that can happen here.
It can be something as simple as an IOException (which is a checked exception) that has to be wrapped in something else.
It can be a parsing issue, it can be a Socket issue, it can be charset issue, etc...
Unlike a normal webapp and how it handles failure, a RequestLog cannot skip its logging.
Ideally, a RequestLog should never call a method that changes the Request or Response state.
There was a problem hiding this comment.
No, I totally get why you catching ALL exceptions.
My only concern is why are you catching Throwable.
The servlet spec may not specify exceptions that can happen but I bet they still are Exceptions and I really, really doubt it says anywhere you should be prepared for Errors there. Errors are usually reserved for some non-recoverable things that JVM may throw (OOM being a good example).
Catching Exception covers both checked exceptions and unchecked (RuntimeException) but not Errors. Catching Throwable adds these Errors which is not always a good idea. Throwables are rarely caught - most likely as a top-level catch-all things in some frameworks just to log stuff that otherwise would go unnoticed etc.
There was a problem hiding this comment.
The Servlet spec operates on/at the Throwable level, not Exception.
You can map Throwables to Error Page mappings.
You get notified of issue with Async via the AsyncEvent which does it via Throwable.
The WriteListener and ReadListener onError methods are Throwable based.
etc ...
There was a problem hiding this comment.
Here's a few things that can happen at the Error level within the Servlet that are non-fatal for the JVM or ServletContext ...
- XML initialization errors (stream, parser, validation, etc) (endpoint specific)
- java.io.IOError with non-socket based connections/connectors (like UnixSocket)
- ServiceLoader and Service provider errors. (endpoint specific)
- java.nio.Charset encode/decode errors. (request specific)
There was a problem hiding this comment.
The logging here in RequestLog should not fail for a fault out of its control.
| } catch (Exception e) { | ||
| // FIXME Why is try/catch required? | ||
| e.printStackTrace(); | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
Ditto here.
The Servlet Spec doesn't specify the exceptions that can happen here.
It can be something as simple as an IOException (which is a checked exception) that has to be wrapped in something else.
It can be a parsing issue, it can be a Socket issue, it can be charset issue, etc...
Unlike a normal webapp and how it handles failure, a RequestLog cannot skip its logging.
Ideally, a RequestLog should never call a method that changes the Request or Response state.
|
@ceki could we get this merged? This prevents updating Jetty to 10.x.x, since logback assumes a method of signature |
|
Any updates on this? This still prevents using recent Jetty versions in combination with Logback |
|
@ceki After revisiting this, I think this PR is the only solution to use Jetty 10.0.x with Logback. As mentioned in [Logback-1610] the correct approach to support Jetty 10.0.x would be to use different modules for Jetty 10.0.x and Jetty 11.0.x. But this isn't possible at the moment (see 2ff2d4e). So my suggestion would be to provide support for Jetty 10.0.x in the Logback 1.2.x branch and remove it from the 1.3.x series in favor of support for Jetty 11.0.x. |
|
I'm out at the moment, I can review this at the end of the month. |
|
Merged into master (logback 1.3.0-bata1). |
HttpServletResponse.getStatus()that was used when the response was committed.AccessEventto not fail logging when theHttpServletRequest.getParameterNames()orHttpServletRequest.getParameter(String)methods are used against unread or even bad request objects.