Dynamic loading of middleware

[shen-tian]

This add a new middleware, dynamic-loader (open to other suggestions), that allows us to modify the middleware stack on the fly. When combined with sideloading, this would allow us to inject middleware that the host JVM is not even aware of at startup.

A possible use case, e.g. CIDER, to connect to an already running nREPL instance, check what middleware are loaded, and if missing cider-nrepl, load it.

Closes #143

API

The new middleware defines three new ops:

• ls-middleware: returns a list of middleware currently loaded.
• add-middleware: add a collection of middleware to the existing stack. This will re-sort all the existing middleware as well as the new ones.
• swap-middleware: replaces the existing stack with a new set of middleware.

Implementation

This is done via a new middleware, dnyamic-loader/wrap-dynamic-loader. There are two interesting bits

Passing of state

An earlier design had the dynamic-loader being a specialised handler, and not a middleware. It was initialised with a list of middleware. This worked quite well, and the handler held its own state of what middleware are active. However it had to be the top most layer, so we can't use any upstream middleware in this handler. In particular, not having access to session made sideloading difficult.

Thus, the current design used an external state atom that stores :handler, the active handler fn built out of all the middleware and :stack, the list of individual active middleware. In server/default-handler, we create this atom, and close over it to return the handler.

We the bind a *state* var in the dynamic-loader ns to this atom when calling the handler. This allows the middleware to modify the global(ish) handler/stack state. (The state is not actually global. It will be created each time we create a new server via default-handler).
This allows us to preserve wrap-dynamic-handler as a 1-arity fn that works well with set-descriptor! and linearize-middleware-stack etc., while effectively passing the state in.

Here's the key lines of code, showing how we create state, bootstrap the stack, and close over the state in the resultant handler:

(defn default-handler
  [& additional-middlewares]
  (let [initial-handler (dynamic-loader/wrap-dynamic-loader unknown-op)
        state           (atom {:handler initial-handler
                               :stack   [#'nrepl.middleware.dynamic-loader/wrap-dynamic-loader]})]
    (binding [dynamic-loader/*state* state]
      (initial-handler {:op          "swap-middlewares"
                        :state       state
                        :middlewares (concat default-middlewares additional-middlewares)}))
    (fn [msg]
      (binding [dynamic-loader/*state* state]
        ((:handler @state) msg)))))

Open to suggestions on this approach. It's a bit loopy, but the weirdness is concentrated in very few places, and makes it easy to reason able this middleware like any other, most of the time.

Working with the sideloader

When updating middleware, this will look to the same (:classloader (meta session)) value that the sideloader uses, just like interruptable-eval. You'll want to enable the sideloader in the "nREPL config" session, update the middleware, and be free to use the middleware without having to deal the sideloader during regular use.

Note that the dynamic-loader doesn't interact directly with sideloader, thus can be before or after it on the stack. However, it does require session to come before it, just for the sideloading case.

Other notes

• The PR is not ready to merge, but does have working tests, including the all important case where we combine this with the sideloader. I'd be keen for comments on the design.
• I'd like to do more on error handling/documentation/tests
• Once we have this in, a path for deferred middleware loading, e.g. the current cider ones, can be simplified a bit:
  • Instead of loading all middleware, and have them manage their own deferred loading, have an "entry point" middleware, which doesn't depend on any of the others, but has a registry of which opts correspond to which middleware.
  • When one of those ops is first called, use the dynamic loader to add the relevant middleware to the stack, then handle the original message.

[bbatsov]

Instead of loading all middlewares, and have them manage their own deferred loading, have an "entry point" middleware, which doesn't depend on any of the others, but has a registry of which opts correspond to which middleware.

Yeah, that's one idea I like very much. One big issue with the deferred loading approach taking currently by cider-nrepl is that it adds a lot of complexity - you have decouple descriptors from the middleware, middleware loading doesn't work the way it would normally work and so on.

[bbatsov]

I'm impressed with the simplicity of the solution! Great work!

I'm also tempted to add a ping middleware to nREPL, as a simple example for a middleware. :D Reminds me of SLIME and SWANK. :D

It'd be nice if you incorporated more of your descriptions from the ticket in the middleware's implementation as docstrings and comments.

[shen-tian]

Thanks for the comments! Made some quick changes in response. I take it you are, in principle, supportive of the design?

I'd like to do a bit more testing of the core capabilities, while I finish off the PR. More comments from the @nrepl team welcome

[shen-tian]

Just hit a good milestone:

1. Started a plain nREPL
2. Connected to it using a client that supported dynamic loading and sideloading
3. Injected cider-nrepl and refactor-nrepl and disconnected
4. cider-connect from Emacs and it was ready to go.

A few new realisations/discoveries:

• Ironically(?) deferred loading in both cider-nrepl and refactor-nrepl made this harder, as the said deferred loading happened outside of an eval or add|swap-middleware, so didn't trigger the sideloader. You can work around this by pre-loading the namespaces though (thus defeating the point of deferred loading). I can think of two ways to make this less hacky:
  1. Introduce some sort of while-sideloading macro, and update relevant middleware to run the deferred loading inside of it. This should be simple (and non-intrusive) enough to do, but raising the question of whether it's desirable to run the sideloader all the time. Deferred loading would most likely happen at an unpredictable time. UPDATE: added the misc/with-session-classloader macro.
  2. Formalise the hack by add an optional :extra-ns slot to add|swap-middleware messages. The client would have to know which (deferred) namespaces goes with which middleware, but it's not hard to figure out. UPDATE: added the extra-namespaces slot.
• Changing the middleware setup destroys any local states contained within the middleware themselves. Only the sessions remain. I came across this because it broke the sideloader's pending atom. I assume we have no guarantee of how stateful middleware in general are, so this might mean we need to be careful with the boot sequence. (This does feel like a disadvantage of the middleware architecture, v.s. an interceptor one: that you can re-assemble interceptors, but the composition of middleware fn's kinda bakes them all together?)
• With all this, it's likely the dynamic loader doesn't remove the need for deferred loading. But it does clarify it's role (optimise startup time) v.s. dynamic loader (post startup configuration).

Will ponder more about this, but quite glad to see this work mostly as envisioned.

[bbatsov]

You'll have to make sure that hotloading still works. I'm always afraid we might break it when fiddling with the classloader, as it doesn't really have any tests.

[shen-tian]

hotloading as in what Pomegranate does?

Dynamic loader capabilities is fairly separate from sideloading, if that's what you are concerned about. This macro is from the changes made in #162, which has its origins in unrepl. Both sideloading + dynamic-loader while sideloading are covered in tests.

I was looking at #113 : is that the kind of breakage you are concerned about? Maybe the the thing to do is to add more tests that we are concerned about?

[bbatsov]

hotloading as in what Pomegranate does?

Yep. It didn't work with nREPL until we did some classloader modifications a while ago, so I'm always wary of us breaking this again.

I was looking at #113 : is that the kind of breakage you are concerned about? Maybe the the thing to do is to add more tests that we are concerned about?

Yeah, we should probably tackle this at some point. Initially we skipped the tests because of the many moving pieces there (hotloading library, build tool).

[shen-tian]

I've made the macro a bit safer. If there's no :classloader under (meta session), then it's pretty much a do block.

Currently, only the sideloader sets :classloader, so it's quite unlikely to make any breaking changes. I'd be happy to chat more about tests/classloader etc. separately though?

[bbatsov]

Looks solid to me! Apart from my small remarks, I think the biggest thing missing is more documentation for client authors.

[bbatsov]

@shen-tian I think we're good to merge. Ideally you should clean up the commit history a bit by squashing related commits together (e.g. - it seems the first 9-10 commits can be reduced to one).

[bbatsov]

Ah, actually you'll also have to run lein docs to regen the ops page.

[shen-tian]

@shen-tian I think we're good to merge. Ideally you should clean up the commit history a bit by squashing related commits together (e.g. - it seems the first 9-10 commits can be reduced to one).

Will do. Was assuming you'll squish :)

[bbatsov]

From the web UI I can only squash all commits in 1. I think it'd better to do manual squashing in this case, as there 3-4 meaningful changes apart from the dynamic loader itself.

[shen-tian]

@bbatsov Squished it down to 6 commits, including regen of the ops page.

[bbatsov]

🎉