search

fzakaria / slf4j-timbre

SLF4J binding for Clojure's Timbre
Eclipse Public License 1.0
94 stars 24 forks source link

Only call .getFileName and .getLineNumber when the caller is not nil. #28

Closed kennyjwilli closed 6 years ago

kennyjwilli commented 6 years ago

When working with Spymemcached I run into a NPE caused by the caller (here) bein nil.

This PR fixes the issue by only including ?:file and ?line in the log! call when the caller is not nil.

rufoa commented 6 years ago

Thanks for the bug report and PR @kennyjwilli! I'll check it out.

I'm pretty sure caller# should never be nil so I wonder if the root cause is actually something else. Do you have a test case you can share that exhibits this problem please?

kennyjwilli commented 6 years ago

The issue is with Datomic and Spymemcached. I created a repo that can easily reproduce the exception: https://github.com/kennyjwilli/memcached-datomic-npe. The README has instructions to reproduce the exception.

rufoa commented 6 years ago

Thanks, that was very helpful!

The root cause seems to be a botched refactoring in Spymemcached: https://github.com/couchbase/spymemcached/blob/730fe4f285887ed074c78da84ef58a85bf8abd4d/src/main/java/net/spy/memcached/compat/log/Log4JLogger.java#L99 (among several other places)

Note that the package name should now be net.spy.memcached.compat.log rather than net.spy.compat.log

One of us should probably make a PR to fix that, but in the meantime I'll come up with a fix for slf4j-timbre to avoid the issue as best I can.

Unfortunately your proposed (cond-> trick won't work here because timbre/log! is a macro that tries to destructure the form at macro expansion time, so it needs a real map not a sexpr that evals to a map.

kennyjwilli commented 6 years ago

Gotcha. I don't know enough about Log4J to put together a good PR. Do you only need to change the package name to net.spy.memcached.compat.log?

Yeah when writing the patch I didn't realize log! was a macro. After having that realization, I used the cond-> approach which removed the exception for me.

rufoa commented 6 years ago

Done the PR, yeah it just needed a search and replace: https://github.com/dustin/java-memcached-client/pull/48

rufoa commented 6 years ago

Pushed v0.3.8 to clojars, please confirm it fixes the issue

kennyjwilli commented 6 years ago

Yep that fixes it! Thanks for the quick fixes.