Problem when :noop-reloading a state in cljc-mode

[krajj7]

I think I found a bug in mount. Example code is below, here's a description how to trigger it:

Using cljc mode in Clojure (not ClojureScript). I make a defstate tagged ^{:on-reload :noop} and start it. At this point the defined var contains a DerefableState, I have to deref it to use it (as expected). However, if I recompile the namespace (re-save the file with some whitespace change and run (clojure.tools.namespace.repl/refresh)), the state var now contains directly what the DerefableState contained before the reload. Calling deref on the state doesn't work anymore. I think that after the refresh it should remain a DerefableState.

This line seems to be causing the "unwrapping" of the DerefableState (it's probably desirable in clj-mode): https://github.com/tolitius/mount/blob/6e848d1ee4dfe7f5f6d9580075fb9c3c7c3e0bd4/src/mount/core.cljc#L191

Example code (:dependencies [[org.clojure/clojure "1.10.0"] [org.clojure/tools.namespace "0.2.11"] [mount "0.1.15"]])

(ns mountbug.core
  (:require [mount.core :refer [defstate start stop]]
            [clojure.tools.namespace.repl]))

(mount.core/in-cljc-mode)

(defstate ^{:on-reload :noop} teststate
  :start "hi")   

(comment
  (start)
  (println (type teststate)) ; before refresh it's DerefableState
  (clojure.tools.namespace.repl/refresh)
  (println (type teststate)) ; after refresh it's String
  )

[tolitius]

I tried to reproduce it, but could not:

(ns issue-104.core
  (:require [mount.core :as mount :refer [defstate]]))

(mount/in-cljc-mode)

(defstate ^{:on-reload :noop} teststate
  :start "hi")

REPL:

boot.user=> (require '[mount.core :as mount]
                     '[issue-104.core :as iss]
                     '[clojure.tools.namespace.repl :as tn])

boot.user=> iss/teststate
#object[mount.core.DerefableState 0x7367eb6b {:status :pending, :val nil}]

boot.user=> (mount/start)
{:started ["#'issue-104.core/teststate"]}

boot.user=> iss/teststate
#object[mount.core.DerefableState 0x7367eb6b {:status :ready, :val "hi"}]

boot.user=> @iss/teststate
"hi"

boot.user=> (tn/refresh)
:reloading ()
:ok

boot.user=> iss/teststate
#object[mount.core.DerefableState 0x7367eb6b {:status :ready, :val "hi"}]

boot.user=> @iss/teststate
"hi"

same thing happens whenever I save the changes to a file / recompile it and mount picks them up (i.e. without an explicit call to (tn/refresh))

I created a gist which you can try. You can change it (i.e. create your own gist) to see if you can reproduce the problem. You clear steps in REPL will help me to see if there is an issue.

[krajj7]

It seems from your REPL output that (tn/refresh) did not pick up any changes, so the bug wasn't triggered.

For some reason clojure.tools.namespace isn't picking up the source directory from build.boot. Calling (tn/set-refresh-dirs "src") before the refresh makes it pick up the changes.

My REPL output (files are identical to your gist) is below. I am also avoiding aliasing the issue-104.core namespace from the REPL, as aliases created in the REPL will become stale after a refresh (documented on https://github.com/clojure/tools.namespace "Warnings for Aliases"), which could obscure the bug.

boot.user=> (require '[mount.core :as mount]
                     '[issue-104.core]
                     '[clojure.tools.namespace.repl :as tn])
nil

boot.user=> issue-104.core/teststate
#object[mount.core.DerefableState 0x318d866a {:status :pending, :val nil}]

boot.user=> (mount/start)
{:started ["#'issue-104.core/teststate"]}

boot.user=> issue-104.core/teststate
#object[mount.core.DerefableState 0x318d866a {:status :ready, :val "hi"}]

boot.user=> @#'tn/refresh-dirs
[]

boot.user=> (tn/set-refresh-dirs "src")
("src")

boot.user=> @#'tn/refresh-dirs
("src")

; make whitespace change in issue-104.core

boot.user=> (tn/refresh)
:reloading (issue-104.core)
:ok

boot.user=> issue-104.core/teststate
"hi"

boot.user=> (type issue-104.core/teststate)
java.lang.String

boot.user=> @issue-104.core/teststate
java.lang.ClassCastException: java.lang.String cannot be cast to java.util.concurrent.Future

[tolitius]

ah.. k. you're right about (tn/refresh). I also mentioned that:

same thing happens whenever I save the changes to a file / recompile it and mount picks them up (i.e. without an explicit call to (tn/refresh))

which is where the {:on-reload :noop} really matters.

In other words: if you work on the project and recompile certain files (or even all the files), mount will pick up the namespace recompilations and will bounce all the states in those namespaces (in the right order) excluding states that have {:on-reload :noop}, preserving the types.

For example, try to remove {:on-reload :noop}, and recompile the file, depending on the editor you use, you'd see something like:

I don't really use (tn/refresh), but I believe it refreshes "too much": i.e. I think it might "refresh" the fact that defstate is not def, and just refreshes it to its dereffed value.

[krajj7]

You were able to reproduce the problem then?

The way I see it is we have two ways of reloading: 1) (require :reload ...), or re-evaluating from the REPL 2) (tn/refresh)

The :on-reload option is applicable in both cases, but with cljc mode it doesn't currently play well with tn/refresh. The key difference here seems to be that tn/refresh also wipes away "defonce" vars before reloading, which is what defstate uses behind the scenes.

Anyhow, I am quite sure this issue exists and can be fixed in mount, rather than in tn/refresh. I have a fork of the latest mount where I fixed it (I just pushed it to github: https://github.com/krajj7/mount/commit/cf47d31a06b99f40c1356f9cf81de617ee90f025), only I did it crudely and most likely broke clj mode (which I currently don't care about). I'm sure a clean fix is possible.

The "fix" is instead of doing the following when noop-refreshing a running state (this line causes the unwrapping): (~'defonce ~state (current-state ~state-name))) https://github.com/tolitius/mount/blob/6e848d1ee4dfe7f5f6d9580075fb9c3c7c3e0bd4/src/mount/core.cljc#L191 do this (but only in cljc mode - not handled in my fork): (~'defonce ~state (->DerefableState ~state-name)) https://github.com/krajj7/mount/blob/cf47d31a06b99f40c1356f9cf81de617ee90f025/src/mount/core.cljc#L187

[tolitius]

this should be fixed in 0.1.16-SNAPSHOT

the problem was here, it should have returned a DerefableState vs. @inst, which it does now.

let me know whether it works.

[krajj7]

I tested and it works well in 0.1.16-SNAPSHOT. Thanks for the fix!

Maybe the cljs variant of the current-state function should be similarly changed (although in cljs you'd have to ns-unmap the state var manually to trigger this problem, since there is no tn/refresh).

https://github.com/tolitius/mount/blob/c5f3e4cdf81d3b632a463a1db19ae3f73c0ad2c9/src/mount/core.cljc#L139-L141