Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sideloader (take 2) #162

Merged
merged 7 commits into from Oct 7, 2019

Conversation

@shen-tian
Copy link
Contributor

commented Sep 24, 2019

Closes #97

This rebases @cgrand's PR #109, and fixes the test. The sideloader itself is from unrepl, so I'm guessing should work well. The bits I'd like to get done still are:

  • Add more tests, at least covering the cases of loading classes and resources.
  • Resolve the question around clojure.lang.Compiler/LOADER a bit. See note later.
  • Add documentation.

How is this meant to be used?

As it stands, the sideloader doesn't work if we (require 'foo.bar) where foo.bar is missing on the nREPL classpath, because it's not hooked into the compiler classloader. See this SO post. So to benefit from the classloader, the code being ran in the client would need to make explicit effort to trigger the sideloader (by explicitly using Thread.currentThread.getContextClassLoader() or similar). This probably works for a tooling "we are going to sideload these classes now" usecase, but not for a general "it just works" one.

unrepl deals with this by wrapping the eval thread with a binding:

https://github.com/Unrepl/unrepl/blob/fa946eef88b0516dab81c8a9b3d8f9fcff06f44b/src/unrepl/repl.clj#L396-L397

I'm not sure it would be safe to do the same in interruptible-eval? This feels like one of those things that might have unforeseen consequences even if the client is not using the sideloader?

If the objective is to get this into nREPL so we can see what we can do with it, then we probably don't need to resolve this now. But some insights from people who's been thinking about this moer would be helpful.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

If the objective is to get this into nREPL so we can see what we can do with it, then we probably don't need to resolve this now. But some insights from people who's been thinking about this moer would be helpful.

More or less. I've never envisioned us to auto-sideload stuff on require - the main usefulness I see for this is two-fold:

  • auto-inject some libraries from nREPL clients - e.g. CIDER can auto-inject orchard if it's sees that cider-nrepl is not currently present and fallback to evaluation of code from orchard to power some of its command. Something like REPLy can sideload a completion library like compliment and so on.
  • auto-injection of some middleware by clients - this is the dream, but it's going to be trickier as we'll also have to recalculate the middleware dependencies between each other after loading some extra middleware.

At any rate - I was only thinking of using sideloading explicitly, without touching the existing evaluation logic. Not sure if @cgrand had a different idea about this.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

All that being said - I can certain envision many benefits if that's working transparently like in unrepl, so the client won't have to do extra checks before deciding if they'd like to sideload something. Then client can simply eval away and missing things would be loaded if possible.

So to benefit from the classloader, the code being ran in the client would need to make explicit effort to trigger the sideloader (by explicitly using Thread.currentThread.getContextClassLoader() or similar).

I can't quite imagine how this is going to look from the client's point of view. Might be easier to just have an explicit sideload message and dispense with the auto-lookup if we have to go down this route.

Another small thing that bothers me with the current design is that the initial message sideloader-start doesn't return done. I think all message should return done. I'm also wondering if it makes sense to have a message to explictly disable sideloading for some session - e.g. after the client has happily loaded everything it needs, so we can avoid potential surprises for the end users related to sideloading.

@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

The (unrepl) API proposed here is very much geared at the "it just works" side loading. The cost is that we'd always need to keep on handle sideloader-lookups on the client side, as once we've started to sideload, we won't know when it will be done. Maybe sideloader-start never returns done is to reflect that? I'm just guessing here.

Maybe a simpler API would be to do an explicit sideload:

;; -> init side loading, along with code that will generate the lookup
{:op      "sideload"
 :id      "1"
 :session "x"
 :code    (quote (require '[orchard.eldoc :as eldoc]
                          '[orchard.info :as info]))}
;; <- lookup for eldoc
{:id      "1"
 :session "x"
 :status  :sideloader-lookup
 :type    "resource"
 :name    "orchard/eldoc.clj"}
;; -> providing eldoc
{:id      "2"
 :session "x"
 :op      "sideloader-provide"
 :type    "resource"
 :name    "orchard/eldoc.clj"
 :content "<base64 package>"}
;; <- ack of provided resource
{:id      "2"
 :session "x"
 :status  :done}
;; <- lookup for info
{:id      "1"
 :session "x"
 :status  :sideloader-lookup
 :type    "resource"
 :name    "orchard/info.clj"}
;; ->
{:id      "3"
 :session "x"
 :op      "sideloader-provide"
 :type    "resource"
 :name    "orchard/info.clj"
 :content "<base64 package>"}
;; <- ack of provided resource
{:id      "3"
 :session "x"
 :status  :done}
;; <- ack of sideloading done
{:id      "1"
 :session "x"
 :value   "nil"
 :status  :done}

Once this is done, we can go back to sending eval. Thoughts?

P.S. I've expanded the test to show how require would work with the current design.

@shen-tian shen-tian force-pushed the shen-tian:sideloader branch 14 times, most recently from e29b090 to 29ee553 Sep 24, 2019
@SevereOverfl0w

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

The idea of unrepl's approach was that you could just send code and unrepl would call you if needed.

That way a client can drop in and out as necessary (eg fireplace did/does this).

@shen-tian shen-tian force-pushed the shen-tian:sideloader branch 2 times, most recently from b3276c6 to 2d239cb Sep 24, 2019
@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

I think I found quite a safe way to deal with the clojure.lang.Compiler/LOADER issue: it only re-binds it if you've already called sideloader-start, so should have a low risk of unexpected breaking changes.

I'm quite happy with where the PR sits, other than documentation. Only other thing @bbatsov suggested (over slack) was a sideloader-stop op, so a client can "safe" nREPL after an initial setup.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

I think I found quite a safe way to deal with the clojure.lang.Compiler/LOADER issue: it only re-binds it if you've already called sideloader-start, so should have a low risk of unexpected breaking changes.

Excellent! Let's proceed with this approach then.

I'm quite happy with where the PR sits, other than documentation.

Same here.

@@ -0,0 +1,100 @@
(ns nrepl.middleware.sideloader

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 24, 2019

Contributor

It'd be nice to document the high-level idea here as well and add "0.7" as the version where the ns was introduced.

@bbatsov bbatsov referenced this pull request Sep 25, 2019
@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

I've been thinking a bit about the docs for the sideloader and I think we need the following:

  • simple overview under Design/Middleware
  • some instructions for tool authors on how to use this. Probably we'll need to put this under a new section - e.g. "Building Clients" or something along those lines.
@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2019

I've been thinking a bit about the docs for the sideloader and I think we need the following:

  • simple overview under Design/Middleware
  • some instructions for tool authors on how to use this. Probably we'll need to put this under a new section - e.g. "Building Clients" or something along those lines.

Makes sense. I know you've been keen for a "guide for client authors" section going. Let's get that started, though I assume that's going to take a while to build up.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

Yep. I plan to tackle the rest of this somewhere in October. It has been on my TODO for quite a while already.

@liquidz liquidz referenced this pull request Sep 29, 2019
@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

I've added some documentation. What should we still do before this can be merged? I feel like the next step for this feature is for a client to try and implement it.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

Yeah, I agree with you. I guess that should be CIDER. :)

So, I'll just have to send from the client jars or clj files that are Base10 encoded, right?

I also finally figured out why @cgrand decided not to send done - I see all the sideloader messages have the same id and I expected they'd have different ids. In the current arrangement the done response definitely doesn't make a lot of sense. Anyways just squash the related commits together and I'll merge this and start playing with it.

@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

I can remove the :done response before we merge? If we ever add a sideloader-stop op, that could be what triggers the :done response for the initial message.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

I like the idea! Let's do this.

@liquidz

This comment has been minimized.

Copy link

commented Oct 5, 2019

@shen-tian Your work is awesome!
Just a quick question.
What response should I send in sideloader-provide when client didn't have a way to provide lookuped type and name?

sideloader-stop will also be usable for aborting sideloader-lookup?

@liquidz

This comment has been minimized.

Copy link

commented Oct 5, 2019

An empty response indicates "resource/class not found.

Oh, I missed this!
@shen-tian Sorry, please ignore the above question.

@shen-tian shen-tian force-pushed the shen-tian:sideloader branch from 4e7f83a to f70f7a6 Oct 7, 2019
@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

Ok. The done response removed again, and commit history cleaned up. :)

@bbatsov bbatsov merged commit 1ac77e8 into nrepl:master Oct 7, 2019
12 checks passed
12 checks passed
ci/circleci: Checking Cljdoc Your tests passed on CircleCI!
Details
ci/circleci: Code Linting Your tests passed on CircleCI!
Details
ci/circleci: Java 11, Clojure 1.10 Your tests passed on CircleCI!
Details
ci/circleci: Java 11, Clojure 1.7 Your tests passed on CircleCI!
Details
ci/circleci: Java 11, Clojure 1.8 Your tests passed on CircleCI!
Details
ci/circleci: Java 11, Clojure 1.9 Your tests passed on CircleCI!
Details
ci/circleci: Java 11, Clojure master Your tests passed on CircleCI!
Details
ci/circleci: Java 8, Clojure 1.10 Your tests passed on CircleCI!
Details
ci/circleci: Java 8, Clojure 1.7 Your tests passed on CircleCI!
Details
ci/circleci: Java 8, Clojure 1.8 Your tests passed on CircleCI!
Details
ci/circleci: Java 8, Clojure 1.9 Your tests passed on CircleCI!
Details
ci/circleci: Java 8, Clojure master Your tests passed on CircleCI!
Details
@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

@shen-tian Your work is awesome!
Just a quick question.
What response should I send in sideloader-provide when client didn't have a way to provide lookuped type and name?

sideloader-stop will also be usable for aborting sideloader-lookup?

To be clear, we are not implementing sideloader-stop for now, as it's not clear whether we'll need it. The circumstance I can imagine it being useful for is if we find sideloader mainly used to do a once-off setup on nREPL in a boot phase of a client, but the altered classloading behaviour causes other difficulties after that. Then there would be a case for reverting to the default classloader.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

Thanks! I'll cut a new alpha right away!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.