replikativ / konserve

A clojuresque key-value/document store protocol with core.async.
Eclipse Public License 1.0
299 stars 25 forks source link

What is the best practice for catching exceptions? #33

Closed johnmcconnell closed 1 year ago

johnmcconnell commented 4 years ago

Example use case:

(require '[clojure.core.async :as async])
(require '[konserve.core :as k])

;; Let's assume the backing storage service is down and
;; unavailable.
;; (async/<!! (k/get storage skey)) will return nil in that case
;; and an exception will be thrown on another thread
(let [r (async/<!! (k/get storage skey))]
  (if (successful? r)
     (continue-with-app)
     (restart)))

Let's suppose your storage service is unavailable, an exception will be thrown one of the threads managed by the core.async thread pool.

I don't see how the existing API's will allow the application code to handle exceptions when interfacing with the key value store. Is there a best practice to handle this situation?

johnmcconnell commented 4 years ago

Another example:

  (async/<!!
   (async/go
     (try
       {:val (async/<! (k/get nil "hello"))}
       (catch Exception ex
         {:ex ex}))))

The snippet above will return {:val nil} due to the semantics of core.async

whilo commented 4 years ago

We follow an extended complementary semantics of go-try and <?. We have a library around the error handling in core.async [1], but since there is no need to push it onto the users of konserve, you just need these two macros that properly handle the errors. <? needs to check for an exception and rethrow and go-try needs to catch and pass it along as a return value such that it does not get lost. That works as long as you do not have concurrent go-routines. For that case we have Erlang style supervisors in superv.async, but this is unfortunately still a bit heavy in terms of metaprogramming and overhead for the general abstractions.

[1] https://github.com/replikativ/superv.async/

We should add that to the documentation, since it is probably not clear. What do you think?

whilo commented 4 years ago

The original concept has been introduced here:

https://swannodette.github.io/2013/08/31/asynchronous-error-handling

But we have needed to extend it to concurrent routines for our networking layer in superv.async and to be able to compile core.async away in the hitchhiker-tree for performance reasons:

https://github.com/replikativ/hitchhiker-tree/blob/master/src/hitchhiker/tree/utils/async.cljc#L34

We are trying to unify these abstractions in superv.async after our pending release of a new konserve version with metadata support. If you are interested in that, let me know. :)

johnmcconnell commented 4 years ago

I'll check in a bit but I suspect the issue needs to be changed in the objects which are implementing: PEDNAsyncKeyValueStore protocol. That or the change needs to happen here: https://github.com/replikativ/konserve/blob/master/src/konserve/core.cljc#L102

whilo commented 4 years ago

konserve should need no changes, otherwise it is a bug in an implementation (if an error is not properly thrown). It should always return an exception object and you need to use <? to properly rethrow exceptions when you take them from the channel.

johnmcconnell commented 4 years ago

I'm in the middle of something so let me go through it, but my current understanding is that: <? relies on the underlying functions/go-routines to catch any exception and add that exception to result channel. If those functions don't do that then <? will received a nil and return that.

whilo commented 4 years ago

Yes, exactly. konserve should return all exceptions on the channel and not leak them anywhere else.

johnmcconnell commented 4 years ago

Main go-lock will "pass through" either nil (if ~@code does not handle exceptions correctly or the exception if <? semantics are respected.

https://github.com/replikativ/konserve/blob/master/src/konserve/core.cljc#L39

Similar to above: https://github.com/replikativ/konserve/blob/master/src/konserve/core.cljc#L108 will pass through any results of -assoc-in (in the example, above, the call assoc-in throws a null pointer exception [because nil doesn't extend the protocol], https://github.com/replikativ/konserve/issues/33#issuecomment-616218255). Technically, this is "unhandleable" how assoc-in is defined but this is not an important problem to solve.

The filestore: https://github.com/replikativ/konserve/blob/master/src/konserve/filestore.clj#L350 will catch any exceptions and add them to result of the channel.

Okay, I guess the current implementation "trusts" the implementors (of the protocol) to handle exceptions correctly. I don't see any "issues".

johnmcconnell commented 4 years ago

Adding an example of catching an exception to the docs is probably a good idea

johnmcconnell commented 4 years ago

I think we can close this issue or update the documentation to include an error handling example (or link): https://github.com/replikativ/konserve/issues/33#issuecomment-616221043

whilo commented 4 years ago

Yes, it is necessary for the backend implementer to catch all exceptions. The new version of the filestore will use the core.async primitives internally itself, which helps with that and increases readability. Ok, we need to add a section about error handling then.