Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

kennyjwilli
Copy link

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
Copy link
Collaborator

rufoa commented Dec 15, 2017

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
Copy link
Author

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
Copy link
Collaborator

rufoa commented Dec 19, 2017

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
Copy link
Author

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
Copy link
Collaborator

rufoa commented Dec 19, 2017

Done the PR, yeah it just needed a search and replace: dustin/java-memcached-client#48

@rufoa
Copy link
Collaborator

rufoa commented Dec 19, 2017

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

@kennyjwilli
Copy link
Author

Yep that fixes it! Thanks for the quick fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants