logging-log4j2 icon indicating copy to clipboard operation
logging-log4j2 copied to clipboard

Issue with Duration Parsing

Open RapidTransit opened this issue 1 year ago • 1 comments

I was working on something unrelated and needed to parse a string like 10s or 10ms, I did a quick search in my project and came up with:

https://github.com/apache/logging-log4j2/blob/b4986fe109d2a37d99b7b167bf50a460c0510c1e/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java#L639C1-L674C6

I'm not sure if it is a bug, but when it finds the first matching suffix it should immediately return, other wise it always falls through to SECONDS for most of the covered cases and the order is slightly messed up as the SECONDS could produce an early match for MINUTES, HOURS, and DAYS.

RapidTransit avatar Nov 11 '23 17:11 RapidTransit

@RapidTransit,

Thank you for catching this bug. As far as I can see, most invocations of this code (especially if the unit ends in s) ends up with an uncaught NumberFormatException.

@rgoers, this is only used in the Kubernetes plugin. Can we replace the entire logic with Duration.parse?

ppkarwasz avatar Nov 12 '23 07:11 ppkarwasz

Since the only users of PropertiesUtil#getDurationProperty has been removed in #2218, I propose to replace the implementation of the method with a simple Duration#parse.

ppkarwasz avatar Mar 27 '24 16:03 ppkarwasz

@ppkarwasz, if there are no other system properties that need a TypeConverter<Duration>, but the Kubernetes module, which is planned to be removed, remove this TypeConverter<Duration> along with it too, please. Note that this implies no TypeConverter<Duration>, not even one using Duration.parse(String).

vy avatar Mar 28 '24 08:03 vy

@vy,

In Log4j Core there is no java.time.Duration converter, there is however an o.a.l.l.c.a.rolling.action.Duration converter created in the well-known spirit of bringing to Java 7 users Java 8 features today!

I can deprecate it and replace its usages with java.time.Duration, but I'd rather do it in a separate PR.

ppkarwasz avatar Mar 28 '24 08:03 ppkarwasz