babashka / pod-babashka-fswatcher

Babashka filewatcher pod.
BSD 3-Clause "New" or "Revised" License
22 stars 3 forks source link

Crashes on error message? #12

Closed aiba closed 2 years ago

aiba commented 2 years ago

I'm getting this stacktrace from fswatcher:

#error {
 :cause "java.lang.String cannot be cast to clojure.lang.Associative"
 :via
 [{:type java.lang.ClassCastException
   :message "java.lang.String cannot be cast to clojure.lang.Associative"
   :at [clojure.lang.RT assoc "RT.java" 827]}]
 :trace
 [[clojure.lang.RT assoc "RT.java" 827]
  [clojure.core$assoc__5481 invokeStatic "core.clj" 193]
  [clojure.core$update invokeStatic "core.clj" 6231]
  [clojure.core$update invoke "core.clj" 6223]
  [sci.impl.vars.SciVar invoke "vars.cljc" 334]
  [sci.impl.analyzer$return_call$reify__9096 eval "analyzer.cljc" 1180]
  [sci.impl.analyzer$return_binding_call$reify__8967 eval "analyzer.cljc" 1098]
  [sci.impl.fns$fun$arity_1__7412 invoke "fns.cljc" 106]
  [babashka.pods.impl$processor invokeStatic "impl.clj" 227]
  [babashka.pods.sci$load_pod$fn__30772 invoke "sci.clj" 119]
  [sci.impl.vars$binding_conveyor_fn$fn__343 invoke "vars.cljc" 154]
  [clojure.core$binding_conveyor_fn$fn__5823 invoke "core.clj" 2047]
  [clojure.lang.AFn call "AFn.java" 18]
  [java.util.concurrent.FutureTask run "FutureTask.java" 264]
  [java.util.concurrent.ThreadPoolExecutor runWorker "ThreadPoolExecutor.java" 1128]
  [java.util.concurrent.ThreadPoolExecutor$Worker run "ThreadPoolExecutor.java" 628]
  [java.lang.Thread run "Thread.java" 829]
  [com.oracle.svm.core.thread.JavaThreads threadStartRoutine "JavaThreads.java" 597]

I think what's going on is that in the error cases, event is a string on this line:

https://github.com/babashka/pod-babashka-fswatcher/blob/76403f153e6255e2ff0062dd9a909c9ec5b14738/watcher/ops.go#L177

And the call to update with a string argument causes the crash.

It seems like this is because in error cases, babashka.WriteInvokeResponse is being called with a json string rather than a Response struct:

https://github.com/babashka/pod-babashka-fswatcher/blob/76403f153e6255e2ff0062dd9a909c9ec5b14738/watcher/ops.go#L122 https://github.com/babashka/pod-babashka-fswatcher/blob/76403f153e6255e2ff0062dd9a909c9ec5b14738/watcher/ops.go#L130

lispyclouds commented 2 years ago

Thanks for raising this @aiba!

@borkdude I think the calls to babashka.WriteInvokeResponse in L122 and L130 should've been babashka.WriteErrorResponse?

I can raise a fix for this and maybe update things too.

aiba commented 2 years ago

If I'm understanding this code correctly, L122 happens when the previous call to WriteInvokeResponse on L115 fails. So that indeed looks like an error case, perhaps to call babashka.WriteErrorResponse.

But it looks like L130 occurs when the fs watcher hits an error, and in this case I think it'd be reasonable to just propagate that to the user's clojure code. I'm guessing the issue is passing a JSON string rather than a struct, since L117 passes the struct directly.

So I'm guessing L130 should be:

babashka.WriteInvokeResponse(message, Response{"error", path, nil, &msg})

Does that seem right?

lispyclouds commented 2 years ago

Yeah looks like that to me too, feel free to raise a PR if you want, I can have a look at it! 😄

lispyclouds commented 2 years ago

Closed in #13

aiba commented 2 years ago

Awesome, any chance you could cut a new release so I can stop depending on my local fix?

borkdude commented 2 years ago

Sure. @lispyclouds Maybe we should take the opportunity to also bump the fs-notify dependency, etc, while we're at it?

borkdude commented 2 years ago

Maybe bump deps in a branch first, to test if nothing breaks.

borkdude commented 2 years ago

If @lispyclouds won't get to it, I can do this in the coming week somewhere.

lispyclouds commented 2 years ago

I should be able to push something in by end of tomorrow hopefully

aiba commented 2 years ago

Thank you!

(I don't suppose there's a way to depend on a pod referenced by :git/url and :sha?)

lispyclouds commented 2 years ago

Released: https://github.com/babashka/pod-babashka-fswatcher/releases/tag/v0.0.3

borkdude commented 2 years ago

Also available from the pod registry now. Thanks all!