juxt / clip

Light structure and support for dependency injection
MIT License
228 stars 15 forks source link

Allow synchronizing system for REPL use. #10

Closed cjsauer closed 4 years ago

cjsauer commented 4 years ago

Hello 👋

I'm running into an exception using the reloaded REPL convenience namespace with the manifold executor. Here is a minimal repro (note I'm using aero):

;; resources/config.edn

{:system-config
 {:executor juxt.clip.manifold/exec
  :components {:noop {}}}}
;; user.clj

(ns user
  (:require [juxt.clip.repl :refer [start stop reset set-init! system]]
            [aero.core :as aero]
            [clojure.java.io :as io]))

(set-init!
 (fn []
   (-> "config.edn"
       io/resource
       aero/read-config
       :system-config)))

When running (start) at the REPL, the following exception is thrown:

Unhandled java.lang.ClassCastException
   class manifold.deferred.SuccessDeferred cannot be cast to class
   clojure.lang.IObj (manifold.deferred.SuccessDeferred is in unnamed module of
   loader clojure.lang.DynamicClassLoader @6ba91ef3; clojure.lang.IObj is in
   unnamed module of loader 'app')

                  core.clj:  217  clojure.core/with-meta
                  core.clj:  675  clojure.core/vary-meta
                  core.clj:  675  clojure.core/vary-meta
               RestFn.java:  442  clojure.lang.RestFn/invoke
                  repl.clj:   25  juxt.clip.repl/start-system
                  repl.clj:   23  juxt.clip.repl/start-system
                  repl.clj:   59  juxt.clip.repl/start/fn
                  AFn.java:  154  clojure.lang.AFn/applyToHelper
                  AFn.java:  144  clojure.lang.AFn/applyTo
                  Var.java:  308  clojure.lang.Var/alterRoot
                  core.clj: 5510  clojure.core/alter-var-root
                  core.clj: 5505  clojure.core/alter-var-root
               RestFn.java:  425  clojure.lang.RestFn/invoke
                  repl.clj:   55  juxt.clip.repl/start
                  repl.clj:   51  juxt.clip.repl/start
                      REPL:   80  user/eval42632
                      REPL:   80  user/eval42632
             Compiler.java: 7177  clojure.lang.Compiler/eval
             Compiler.java: 7132  clojure.lang.Compiler/eval
                  core.clj: 3214  clojure.core/eval
                  core.clj: 3210  clojure.core/eval
                  main.clj:  437  clojure.main/repl/read-eval-print/fn
                  main.clj:  437  clojure.main/repl/read-eval-print
                  main.clj:  458  clojure.main/repl/fn
                  main.clj:  458  clojure.main/repl
                  main.clj:  368  clojure.main/repl
               RestFn.java:  137  clojure.lang.RestFn/applyTo
                  core.clj:  665  clojure.core/apply
                  core.clj:  660  clojure.core/apply
                regrow.clj:   18  refactor-nrepl.ns.slam.hound.regrow/wrap-clojure-repl/fn
               RestFn.java: 1523  clojure.lang.RestFn/invoke
    interruptible_eval.clj:   79  nrepl.middleware.interruptible-eval/evaluate
    interruptible_eval.clj:   55  nrepl.middleware.interruptible-eval/evaluate
    interruptible_eval.clj:  142  nrepl.middleware.interruptible-eval/interruptible-eval/fn/fn
                  AFn.java:   22  clojure.lang.AFn/run
               session.clj:  171  nrepl.middleware.session/session-exec/main-loop/fn
               session.clj:  170  nrepl.middleware.session/session-exec/main-loop
                  AFn.java:   22  clojure.lang.AFn/run
               Thread.java:  830  java.lang.Thread/run
SevereOverfl0w commented 4 years ago

Hmm. Makes sense. I don't think I intended for the repl namespace to work outside of the synchronous executor.

It would need to participate in the executor somehow. That might be possible, but it's probably unexpected to have an async reset workflow.

SevereOverfl0w commented 4 years ago

One option might be a "synchronize" option to the repl code.

That way we avoid executors getting clever about sometimes synchronizing.

Technically, manifold could synchronize the whole thing by deref'ing and I think that would cover most use cases. That wouldn't work for clojurescript very well though, with the promesa library.

SevereOverfl0w commented 4 years ago

To be completely explicit too, manifold could have its own repl namespace. That makes automated tooling more difficult though.

cjsauer commented 4 years ago

I think I like the approach of manifold having its own separate repl namespace. Some sort of synchronization would be great. Maybe a dev-only component could be injected into the system config whose sole purpose is to depend on all other components. That would give the repl ns something explicit to sync against.

Right now my workaround is just rolling my own start/stop functionality, and not worrying about syncing.

(def system-config nil)
(def system nil)

(defn start []
  (alter-var-root #'system-config (constantly (config/dev-system)))
  (alter-var-root #'system (constantly @(clip/start system-config)))
  (keys system))

(defn stop []
  (alter-var-root #'system
                  (fn [s]
                    (when s
                      @(clip/stop system-config s)))))

(defn reset []
  (stop)
  (refresh :after 'user/start))

This is pretty sketchy, but it works. My components all log their start/stop status to stdout, so when I run (start) I just "manually" wait for everything to spin up before trying to poke at the system.

SevereOverfl0w commented 4 years ago

Do you think it's useful to have start be async?

cjsauer commented 4 years ago

Not personally, no, but my use-cases are limited. I'm not using clip in cljs (browser), for example, where it's prudent to let go of the render thread asap. Async start makes more sense in that context. Currently I'm only using clip for backend work, which means start is the "last" thing I call before just waiting on @(promise) indefinitely, as in your readme example.

SevereOverfl0w commented 4 years ago

Apologies, I mean for the REPL only. I'm thinking that async (reset) makes no sense. I realize now I missed a @ in your example, I thought your (repl/start) was async.

cjsauer commented 4 years ago

Agreed, I don't think it makes sense for (reset) to be async at the REPL.

SevereOverfl0w commented 4 years ago

I would welcome a PR which:

Allows :juxt.clip.repl/deref in the system, which is a function/symbol which allows for "synchronizing" the system between runs. Most often it will be deref. If it is a symbol it should be requiring-resolve'd. If the symbol has no namespace, it should be resolved in clojure.core.