search

qos-ch / reload4j

reload4j is a drop-in replacement for log4j 1.2.17
Apache License 2.0
148 stars 22 forks source link

AppenderAttachable contract changed by performance improvement #52

Closed DanielThomas closed 1 year ago

DanielThomas commented 2 years ago

appenderList is now never null:

https://github.com/qos-ch/reload4j/commit/63ff036d11b14767365197a79606cb6df6102c3a#diff-e36ce72be14b72b7656aa689c45e327e1c84bedccd4191fc54e959322b494519R35

Which means that code that used getAllAppenders() == null to decide if appenders were not configured now doesn't work ( we have programmatic configuration of loggers happening). It should be initialized here to keep the existing nullability behaviour:

https://github.com/qos-ch/reload4j/commit/63ff036d11b14767365197a79606cb6df6102c3a#diff-e36ce72be14b72b7656aa689c45e327e1c84bedccd4191fc54e959322b494519L48

ceki commented 2 years ago

@DanielThomas Could you test for array size instead?

DanielThomas commented 2 years ago

@ceki Maybe, this is legacy code I was hoping to avoid touching, we haven't released it in 5 years and is dependent on being able to tell if appenders have been registered:

https://github.com/Netflix/blitz4j/blob/d03aaf4ede238dc204a4420c8d333565f86fc6f2/src/main/java/com/netflix/blitz4j/AsyncAppender.java#L165-L188

There are still appenderList == null checks in that class, so assumed this was an oversight. Was there a provblem you were trying to avoid by having the field initialized by default?

DanielThomas commented 1 year ago

Will fix downstream, thanks.