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

Implement an EDN transport #60

Open
bbatsov opened this Issue Oct 22, 2018 · 10 comments

Comments

Projects
None yet
2 participants
@bbatsov
Copy link
Contributor

bbatsov commented Oct 22, 2018

Having an EDN transport would simplify the lives of people using Clojure/ClojureScript/Java clients for nREPL. It should be pretty easy to create a transport based on the default bencode transport that simply uses EDN instead of bencode. Here's the implementation of the bencode transport as a reference:

(defn bencode
  "Returns a Transport implementation that serializes messages
   over the given Socket or InputStream/OutputStream using bencode."
  ([^Socket s] (bencode s s s))
  ([in out & [^Socket s]]
   (let [in (PushbackInputStream. (io/input-stream in))
         out (io/output-stream out)]
     (fn-transport
      #(let [payload (rethrow-on-disconnection s (bencode/read-bencode in))
             unencoded (<bytes (payload "-unencoded"))
             to-decode (apply dissoc payload "-unencoded" unencoded)]
         (merge (dissoc payload "-unencoded")
                (when unencoded {"-unencoded" unencoded})
                (<bytes to-decode)))
      #(rethrow-on-disconnection s
                                 (locking out
                                   (doto out
                                     (bencode/write-bencode %)
                                     .flush)))
      (fn []
        (if s
          (.close s)
          (do
            (.close in)
            (.close out))))))))

The best thing about this transport is that it requires no extra deps, so it can be easily bundled with nREPL itself.

@dotemacs

This comment has been minimized.

Copy link
Contributor

dotemacs commented Oct 29, 2018

Let me nudge this along...

Since bencode is used, it needs to:

  1. read the input
  2. decode the input so that it can be used

With EDN it would be something along these lines instead:

;; require EDN: [clojure.edn :as cljedn]

(defn edn
  "Returns a Transport implementation that serializes messages
   over the given Socket or InputStream/OutputStream using EDN."
  ([^Socket s] (edn s s s))
  ([in out & [^Socket s]]
   (let [in (PushbackInputStream. (io/input-stream in))
         out (io/writer out)]
     (fn-transport
      #(cljedn/read in)
      #(rethrow-on-disconnection s
                                 (locking out
                                   (doto out
                                     (.write %)
                                     .flush)))
      (fn []
        (if s
          (.close s)
          (do
            (.close in)
            (.close out))))))))

Am I going in the right direction here?

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Oct 29, 2018

Yeah, seems to me that you're moving in the right direction. The situation with EDN is much simpler because there's nothing really to encode/decode.

Since bencode is used, it needs to:

I think I'm missing something. Bencode won't be used at all in an EDN transport.

@dotemacs

This comment has been minimized.

Copy link
Contributor

dotemacs commented Oct 29, 2018

Yeah, seems to me that you're moving in the right direction. The situation with EDN is much simpler because there's nothing really to encode/decode.

Great, thanks.

Since bencode is used, it needs to:

I think I'm missing something. Bencode won't be used at all in an EDN transport.

I mean that it's used in the bencode function you pasted in the issue's description.

@dotemacs

This comment has been minimized.

Copy link
Contributor

dotemacs commented Oct 29, 2018

I made it into a commit here. I'll look at what else needs to be added for this to become a proper pull request.

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Oct 29, 2018

Looking forward to it!

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Oct 30, 2018

Btw, you can test the result like this:

(with-open [conn (nrepl/url-connect "edn://localhost:8000/repl")]
    (-> (nrepl/client conn 1000)
        (nrepl/message {:op "eval" :code "(+ 2 3)"})
        nrepl/response-values))

That assumes you've named this protocol edn like this:

(.addMethod nrepl/url-connect "edn" #'edn)

Maybe a name with nrepl+edn wold be better. I'll leave to you to figure out those details.

@dotemacs

This comment has been minimized.

Copy link
Contributor

dotemacs commented Oct 30, 2018

Thanks for the nudge...

@bbatsov

This comment has been minimized.

Copy link
Contributor Author

bbatsov commented Oct 30, 2018

Any time! 😉

@dotemacs

This comment has been minimized.

Copy link
Contributor

dotemacs commented Oct 30, 2018

I think that I've done it, but there is a failing test...

The change is here

Just dealing with this test failure:

lein test :only nrepl.core-test/transports-fail-on-disconnects

FAIL in (transports-fail-on-disconnects) (core_test.clj:409)
Ensure that transports fail ASAP when the server they're connected to goes down.
expected: (thrown?
           SocketException
           (transport/send transport {"op" "eval", "code" "(+ 5 1)"}))
  actual: nil

Still WIP, so I'll clean it up a bit before making it a proper PR.

@dotemacs

This comment has been minimized.

Copy link
Contributor

dotemacs commented Oct 30, 2018

Solved the failing test issue, it was just the way I required things.

I'll open a PR to move the discussion there.

@dotemacs dotemacs referenced this issue Oct 30, 2018

Open

Added EDN transport #67

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment