Expose exception to user as :throwable

taoensso / nippy

The fastest serialization library for Clojure

https://www.taoensso.com/nippy

Eclipse Public License 1.0

1.04k stars 60 forks source link

Expose exception to user as :throwable #53

Closed kul closed 9 years ago

kul commented 9 years ago

This is related to #51. The root cause of the issue was that clojure datastructures implement Serializable interface without having a serialVersionUID. I had filed a JIRA for this earlier http://dev.clojure.org/jira/browse/CLJ-1327. With this change the error becomes evident to user of nippy which is otherwise ignored.

{:nippy/unthawable "org.Dummy"
 :type :serializable
 :cause "clojure.lang.Keyword; local class incompatible: stream classdesc serialVersionUID = 412129
1061259229960, local class serialVersionUID = -6496198289950458564"}

ptaoussanis commented 9 years ago

Hi kul, this looks good to me - but I'd prefer that the message be under a :message key since cause already has a common + different meaning in the context of exceptions.

Come to think of it, it may be best to just attach a :throwable e entry, then folks can do whatever they like with it - incl. extract the msg/cause/stacktrace/etc.

Do you agree with my thinking here?

kul commented 9 years ago

Yes this sounds good to me. It could be used to get both messages as well as stack trace. Should I also make changes to incorporate this at few other places too?

ptaoussanis commented 9 years ago

Sure, that sounds good. Anywhere that's catching and returning a map should probably include a :throwable map entry.

Thanks!

kul commented 9 years ago

How are you running the tests, they seem to be failing with lein test/lein test-all. Which also led me to a code path which i think was not tested before and ex-info was passed wrong number of arguments. I have made the changes to commit to reflect them.

ptaoussanis commented 9 years ago

lein test-all should work (just confirmed) - what failure are you seeing?

ptaoussanis commented 9 years ago

And good catch on the bad ex-info call: https://github.com/ptaoussanis/nippy/commit/6b58dfa991aa0fce43dc0c509ea89fe44d6570f5

kul commented 9 years ago

Here is the full log. https://www.refheap.com/92633 after above changes.

ptaoussanis commented 9 years ago

Thanks, those should be passing (they're passing on my side). Just engaged in something today, but I'll try take a proper look tomorrow & get back to you.

If you'd like to try debug in the meantime, I'd start by checking what compressor-id is coming up since that's what is triggering the :auto not supported error.

kul commented 9 years ago

Ok i will try to have a crack at it. Thanks.

kul commented 9 years ago

Ok I had a few observations

test-auto passes all tests and benchmarks.
installing the artifact into local repo and running the same functions as the tests i.e. (comp (thaw..) (freeze..)) with passwords also works correctly from a project using it.

Since i do not use expectations myself I am not sure why it is failing for one time tests and works fine for continuous testing. I will test some more later. Meanwhile this PR should be good to merge imo.

Thanks

ptaoussanis commented 9 years ago

Commit looks good to me, thanks Kul!