CLJS function instrumentation is never removed, even after refreshing browser

[danielcompton]

I found that once you add instrument a CLJS function with a Malli function schema, the function stays instrumented until the ClojureScript build is restarted (and maybe cleaned).

I've created a minimal reproduction with steps here: https://github.com/danielcompton/quickstart-browser.

I think the issue is that Malli's instrumentation state is being tracked in atoms and isn't being cleaned up when recompiling.

I'm not 100% sure but it feels like this may be unsafe in general with the shared AOT cache: https://clojurescript.org/news/2018-03-28-shared-aot-cache.

[dvingo]

Thanks for the reproduction.

I think this is due to a combination of unstrument not being applied for cljs (on purpose) see:
a242e21

and the instrumented functions being tracked inside of a defonce'd atom:

malli/src/malli/instrument/cljs.cljs

Line 5 in 3599fbd

(defonce instrumented-vars (atom {}))

I won't be able to dig into this soon, but I believe changing the defonce to a def should resolve this issue if you want to try that.
I don't believe this will affect instrumentation otherwise, but that will need to be confirmed.

Unstrument was removed, as noted in the above commit, because it also led to stale code regarding hot-reloading.

[dvingo]

Oh, but that wouldn't explain why browser refresh doesn't fix things - I think the issue is that the instrumented functions are stored in the compiler state, which happens here:

malli/src/malli/core.cljc

Line 2426 in 49cce6a

;; so it is visible in the (function-schemas :cljs) map at macroexpansion time.

and here for metadata declared schemas:

malli/src/malli/instrument/cljs.clj

Line 39 in 49cce6a

(m/-register-function-schema! ns simple-name schema* metadata :cljs identity)

I think the solution will not be such a quick fix as stated in the first comment. This came up in the past
#695 (comment)
the relevant part being:

So the solution I'm thinking is to have the macro emit code which does all of the work at runtime - the critical part being making sure the invocation of (malli.core/function-schemas :cljs) happens at runtime - which will then be sure to use the most up-to-date registered function schemas.

This will require updating the macro to perform this loop

malli/src/malli/instrument/cljs.clj

Line 94 in 49cce6a

(reduce

in cljs instead of clj.

[dvingo]

For a quick fix you should be able to use unstrument either in a REPL or eval'd in a namespace and then subsequently removed.

[danielcompton]

For a quick fix you should be able to use unstrument either in a REPL or eval'd in a namespace and then subsequently removed.

I'm using md/stop! which calls unstrument, I can't seem to get it to remove the state no matter where I put the call. Have you been able to make it work?

[dvingo]

I am not a dev machine, so haven't tried it, but wanted to reply.

I'm afraid I was mistaken, it looks like unstrument doesn't affect malli.core/-function-schemas* which is what the instrumentation code loops over.

malli/src/malli/core.cljc

Line 2399 in 49cce6a

(defonce ^:private -function-schemas* (atom {}))

I think that's the source of the bug. I won't be able to dig in further for at least another week though.

However, I think the solution is to have unstrument remove the functions from that atom, if someone wants to attempt a fix.

[ingesolvoll]

his will require updating the macro to perform

@dvingo If I understood this correctly, this might be the fix to our other problem where function identity is broken by the instrumentation wrapping function. A macro might be able to do a better job in instrumenting without changing the original structure too much.

Thanks you so much for elaborating on this, I'll try to see if I can come up with something myself.

[dvingo]

Unless I'm confused by what your issue is, I'm not sure the function identity issue can be solved, from the definition of instrumentation: replacing one function with another.
from the docstring:

malli/src/malli/core.cljc

Line 2434 in 49cce6a

"Takes an instrumentation properties map and a function and returns a wrapped function,

[dvingo]

@danielcompton I had some time to look into this.

Have a fix here #745
if you want to try running my PR branch of malli against your app to test it out.

[dvingo]

@danielcompton I think this can be closed now, right? I just tried to repro again against malli 0.10.1 and it works well now.