search

plumatic / schema

Clojure(Script) library for declarative data description and validation
Other
2.4k stars 256 forks source link

Don't catch Throwable on the JVM #412

Closed gfredericks closed 5 years ago

gfredericks commented 5 years ago

I don't think catching Throwable is a good thing for schema to be doing.

This came up when I was running a repl thread that was doing lots of schema completer work, and I couldn't (.stop thread) it. Presumably schema was catching the ThreadDeath exception and turning it into a completer error.

I think that kind of thing is indicative of the difference between catching & swallowing Exception vs Throwable.

gfredericks commented 5 years ago

Another good example of this is an out of memory exception, or a stack overflow; I wouldn't want schema catching those.

gfredericks commented 5 years ago

I suspect most use cases that this was aimed at should be instances of Exception, but it's hard to be confident that all of them are. So that's the risk of making this change.

w01fe commented 5 years ago

Yeah, this is a dirty but Exception is also often not broad enough (e.g. AssertionError is not an Exception). The right thing is probably something like Scala's NonFatal https://github.com/scala/scala/blob/2.13.x/src/library/scala/util/control/NonFatal.scala#L42 but I don't know if there is a similar thing in Clojure land? I guess we could just copy those cases here... thoughts?

gfredericks commented 5 years ago

I didn't know about NonFatal -- yes, I think that approach is totally doable, and also conservative w.r.t. backwards-compatibility in that it keeps the present behavior by default.

(I'm not confident that this would have been the best approach from the beginning, but backwards compatibility is important, and this approach solves my problem, so I'm happy with it)

I'll make that change and then merge & release sometime tomorrow probably.

w01fe commented 5 years ago

Yeah, I'm not sure what the best thing overall is if we were starting over. But agree that given where we're at, the NonFatal approach should be a strict improvement. Thanks!

gfredericks commented 5 years ago

Closing this because #413