search

taoensso / timbre

Pure Clojure/Script logging library
https://www.taoensso.com/timbre
Eclipse Public License 1.0
1.44k stars 171 forks source link

Fix spit-appender lock for graalvm #334

Closed ericdallo closed 3 years ago

ericdallo commented 3 years ago

When compiling a app using spit-appender, graalvm complains about a lock from monitor-enter/exit that does not matches. Log: https://pastebin.com/g9wqxqbv

This looks the same issue clojure.core had on its defmacro locking on CLJ-1472, since the macro is fixed, we can use it instead of manually calling monitor-enter/exit

Thank you for the help on this @borkdude

borkdude commented 3 years ago

@ericdallo I think you should keep respecting the locking? boolean unless @ptaoussanis thinks otherwise.

ericdallo commented 3 years ago

Done @borkdude, not sure the code is good though

borkdude commented 3 years ago

@ericdallo LGTM

ericdallo commented 3 years ago

@ptaoussanis this seems to fix my issue here: https://github.com/clojure-lsp/clojure-lsp/pull/267 :)

ericdallo commented 3 years ago

Hey, @ptaoussanis could you review/merge this, please?

ptaoussanis commented 3 years ago

Should now be fixed on master, pushing release shortly - thanks!

ericdallo commented 3 years ago

Hey @ptaoussanis I tested and it seems your fix on master doesn't work :/ Here is the stack trace: https://pastebin.com/21BByhhX

ericdallo commented 3 years ago

Any reason why not use clojure locking macro (used in this PR) instead of manually using monitor-enter/exit ?

borkdude commented 3 years ago

I think the verifier might have trouble analyzing this since there is a condition involved. Just using the locking macro should solve this as in @ericdallo's PR.

ptaoussanis commented 3 years ago

Apologies! The proposed fix unfortunately breaks the intended locking behaviour (the object to lock against needs to be shared), and I was under the impression from Clojure CLJ-1472 that pulling the lock into a local let would be sufficient + a smaller change.

Would like to understand a little better what the cause of the Graal problem is exactly. Could someone show me what I'd need to do to reproduce the verification error locally? (Have zero experience with Graal).

Thanks!

ericdallo commented 3 years ago

@ptaoussanis I think just replacing the manual lock to a locking clojure would be enough, but if you want to test it manually, you can clone clojure-lsp on this branch: https://github.com/clojure-lsp/clojure-lsp/pull/267/files and run ./graalvm/native-compile.sh, you will need graalvm 11 21.0 installed too.

borkdude commented 3 years ago

@ptaoussanis To test this, you can set up the hello world project as described here:

https://github.com/lread/clj-graal-docs/blob/master/doc/hello-world.md

And then use timbre to log something in the main function.

ptaoussanis commented 3 years ago

@borkdude's comment here was correct (and thanks for helping clarify):

every invocation of the function needs to be locked on the same object

Unfortunately the latest PR re-introduces a possible deadlock: https://github.com/ptaoussanis/timbre/pull/330. We need to be careful about not unnecessarily changing the semantics of what happens under lock.

ericdallo commented 3 years ago

@ptaoussanis is the PR ok now? Should I change anything more?

ptaoussanis commented 3 years ago

Thanks for the updated PR, looks good 👍

com.taoensso/timbre {:mvn/version "5.1.2-SNAPSHOT"} is on Clojars now. Could you please give it one last check and if you're satisfied, I'll cut a stable release?

BTW the issue with Timbre pulling in transitive :dev dependencies should now also be resolved, so you should be able to remove any exclusions that you've added.

ericdallo commented 3 years ago

Perfect! Sure, I'll try right now

ericdallo commented 3 years ago

Got this error now: image

It seems to be this issue reported by @borkdude: https://github.com/oracle/graal/issues/1613#issuecomment-521592548

ericdallo commented 3 years ago

So is there any way to fix that including taoensso.encore source code? how did you fix it exactly @borkdude?

borkdude commented 3 years ago

@ericdallo As you can read in that issue: on macOS CI it helped to bump Xcode. Not sure why this helped.

ericdallo commented 3 years ago

Yeah, I'm compiling on NixOS :/ Not sure it'll happen on the CI too, but seems to happen with that snapshot from @ptaoussanis

borkdude commented 3 years ago

Let's not bother @ptaoussanis with this. The locking issue is now solved.

ericdallo commented 3 years ago

Yeah, besides that, I could not confirm if it fixed @ptaoussanis but I could confirm the exclusions are not necessary anymore indeed.

ptaoussanis commented 3 years ago

Merged manually, thanks again!

ericdallo commented 3 years ago

just FYI I really don't know why, but the 5.1.2 fixes the above issue 🤷🏻‍♂️ while the 5.1.2-SNAPSHOT doesn't, so that's good news :D