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

Adding EDN transport, take 2 #135

Merged
merged 26 commits into from Mar 26, 2019

Conversation

Projects
None yet
4 participants
@shen-tian
Copy link
Contributor

commented Mar 24, 2019

My attempt to finish what @dotemacs started in #67. Closes #60

I followed the discussion under the previous PR, which provided some great sign-posts. What I did, in rough order:

  • Rebased @dotemacs's commits onto master,
  • Pending the discussion that started in the last PR, and I'm sure will continue, around how EDN transport will work, I've taken a permissive approach of aggressively converting all keywords (keys or values) to strings within the transport for both send and receive. This may not be what we ultimately want to do, but allowed rest of the work to continue.
  • Update: we can do without this now! Hacked around a "can't read UUID as a number" bug, which only happened for UUIDs started with a digit. The workaround is all our rand UUIDs start with a letter. This can't go into production, of course. Ideas? @pfeodrippe , you talked a bit about the bug here
  • Converted all the test to use a "normalized" form of :status, a set of keywords. The converting to sets in NB because the EDN transport preserves the set structure, whereas bencode did not. The keywordizing might not be as important, pending the previous question. This is mostly testing code though, so not a design decision.
  • The biggest breakthrough was this commit. I don't know why this works, but it fixed most of the printing related tests. *print-readably* is being left in an odd state by the print middleware, as far as I can gather. @cichli might have some insight?

There's still quite some work left to do, but I think the problem's back is broken.

To finish this, from me:

  • Deciding how permissive/strict the EDN transport should be, and update accordingly.
  • Figuring out the underlying reason for the UUID reading bug, and remove the workaround.
  • Figuring out the underlying reason for needing the *print-readably* binding.
  • Fixing the remaining test or two, Update: fixed and have a look at the assertion that was removed here
  • Some manual testing with a REPL implementation?

In the meanwhile, comments/ideas welcome.

Checklist

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings
  • You've updated the changelog(that only applies to user-visible changes)

dotemacs and others added some commits Oct 29, 2018

Added EDN transport
With the introduction of EDN transport, it theoretically simplifies
the lives of all the clients since they don't have to use Bencode, but
can use the native EDN format.

When reading the EDN payload, it has to be cast to PushbackReader
because of:

https://dev.clojure.org/jira/browse/CLJ-1611
https://groups.google.com/forum/#!topic/clojure/3HSoA12v5nc
Pull in the changes from #84
The changes below were done by pfeodrippe while working on adding the
transit transport and they are useful for this implementation also, so
no need for duplication.
Convert messages to keywords
Since this is EDN messages are using keywords instead of string keys.
That is why I'm converting all the messages to keywords, that way all
the tests are staying mostly the same.
Still WIP
Catching some exceptions and trying to handle them, but there are
still issues
Change send in FnTransport to use custom version of stringify-keys
The tests are now passing... if we skip the new transport
Catching RuntimeException in transport as EOF.
Will be stricter, but let's see if this works for now

@shen-tian shen-tian changed the title Edn transport Adding EDN transport, take 2 Mar 24, 2019

@arichiardi

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Wow awesome job, thank you! Following from far🍿

@shen-tian shen-tian force-pushed the shen-tian:edn-transport branch from 1f9ebc5 to 8029c5e Mar 25, 2019

@shen-tian shen-tian force-pushed the shen-tian:edn-transport branch from 8029c5e to 6410177 Mar 25, 2019

@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Managed to fix the last consistently breaking test! Gonna make a list of things that sporadically fail to squish:

  • edn-transport-communication - nrepl.edn-test Something to do with not closing a socket/releasing the port properly? example
  • interrupt-load-file - nrepl.core-test Also seen here, so don't think it's related to this PR. example
  • test-url-connect - nrepl.core-test This happend on the bencode transport. I assume it's a timeout sensitivity things? example

Fails but not in scope

  • streamed-printing - nrepl.core-test - As reported in #132
Restoring an assertion in return-on-incomplete-expr.
The original version seems to work. Left note about avoiding a
regex exception in the course of testing
@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Great progress! I'll review the code a bit later.

Are things behaving well when you test them manually in a REPL?

Show resolved Hide resolved src/clojure/nrepl/core.clj Outdated
Show resolved Hide resolved src/clojure/nrepl/core.clj Outdated
Show resolved Hide resolved src/clojure/nrepl/misc.clj Outdated
@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@bbatsov I haven't tried yet. I am a bit worried about all the dynamic print related vars. Not sure how much this is just a feature of nREPL v.s. something odd going on elsewhere in the system. Will try that later. In the meanwhile:

On the EDN transport protocol:

What do we accept, and what do we guarantee in sending back? The default solution is that we just stick to the implicit (because it's never been exposed?) internal protocol for both, e.g.

{:op :eval :code "(+ 2 2)"}

Pros:

A major plus for EDN transport is to serve Clojure(Script) clients, which will all be able to speak EDN natively. Also, we can always be strict now, and relax things later, rather than become stricter and causing breaking changes.

Cons:

Cons: This will take quite some documenting/checking, as it's more prescriptive than the simpler bencode transport, which is stringify-everything. The would extend to all the middleware too, as they'd all be able to speak EDN now. This is kind of like having everyone seeing colour for the first time... and immediately requiring colour coordinated uniforms. I'm not even sure this is possible without a coordinated effort with all middlewares, as in essence, we are requiring more detail in the specification of all of them. Overall, this might not be possible in practice? @cgrand mentioned Postel's law in #67. Maybe this is more applicable to third party middlewares than clients?

Alternatives:

Stringy-fy everything:

Requires not much more specification than what already exists. We'd need to specify :status is either a string or a set of strings etc., but I'm not aware of any other EDN structures that we are now exposing from the internals, that was previously hidden behind the simplicity of bencode.

The PR as it stands follows this, though more to facilitate discussion, (and having passing tests as a starting point) than it being my preference.

Permissive on input, strict on output

Think we'll still have the same problem as before, as some stuff that's pretty much implementation detail now might become the defacto spec, possibly without notice to middlware authors.

Strict on everything we know about, be relaxed on the rest?

Between cider-nrepl refactor-nrepl piggieback and the build-in middlewares make up the bulk of real life ones, we can just define what they do, and leave "third-party" middleware to deal with the specification problem?

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

The middlewares don't actually decide what type of data to return - that's entirely up to the transport. In practice all middlewares kept to the simply data structures supported by bencode (namely lists and maps of primitive data types like strings and numbers) and we can't really change this, although we can have some middleware returning more complex data structures when they using the EDN transport.

We will decide this down the road, but for starters I'd just replicate what we have right now. Bencode requests and responses couldn't contain keywords because bencode can't represent them, but with EDN that's obviously not a problem, so I'd just move the current stringification to the bencode transport implementation (as opposed to being global as it is now). Then every transport can individually decide what structure of requests and responses it requires. I doubt any client would aim to support different transports (as this is a bit pointless), so this differences won't really be felt most of the time.

Does this make sense to you?

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

This is where we messed up in the past https://github.com/nrepl/nrepl/blob/master/src/clojure/nrepl/transport.clj#L40 It was marked to be fixed a long time ago, but with only one transport it wasn't that big of a deal anyways.

@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

The middlewares don't actually decide what type of data to return - that's entirely up to the transport. In practice all middlewares kept to the simply data structures supported by bencode (namely lists and maps of primitive data types like strings and numbers) and we can't really change this, although we can have some middleware returning more complex data structures when they using the EDN transport.

Technically true, yes, but the method you favour, if I understand, is for the EDN transport to be transparent? This would expose (previously unexposed) workings of the middleware to the outside. This was hidden by the limitation of bencode previously. This is, in practice, making this canonical.

For example, I think as it stands, {:op :eval :code "(+ 2 2)"} is not actually valid with a transparent transport. The version that would work is {:op "eval" :code "(+ 2 2)"}. This was previously discussed here but not resolved.

This contradicts the published documents but didn't matter, because the bencode stripping and keywordizing at the end hid that detail before.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

For example, I think as it stands, {:op :eval :code "(+ 2 2)"} is not actually valid with a transparent transport. The version that would work is {:op "eval" :code "(+ 2 2)"}. This was previously discussed here but not resolved.

Yeah, this is a good point. Probably we should make the EDN transport permissive as well on the request level, but the conversion of keywords should be done only in the transport itself and we should rely on the top-level conversion the way the bencode transport does it. Btw, I do think that internally we should be converting everything to keywords, not the other way around.

@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

I hear you on changing all the keywordize/stringify into transports. Should we tackles it in this PR?

We can be permissive on the request in side... but would still need to decide what we emit though. I can go at sketching out a (cloujure.)spec in spirit of what you are describing. (btw, how do you feel about having a spec for response + requests? useful?)

Do you think there's room to release this feature as an "alpha/beta/experimental" feature, i.e. might change the format a bit? I'm a bit weary of a yak shaving mission where we have to be way more sure of the format than we need to be.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

I hear you on changing all the keywordize/stringify into transports. Should we tackles it in this PR?

No, we can do this later.

We can be permissive on the request in side... but would still need to decide what we emit though. I can go at sketching out a (cloujure.)spec in spirit of what you are describing. (btw, how do you feel about having a spec for response + requests? useful?)

That's a pretty cool idea! Specing the requests and responses would be super useful in the long run.

Do you think there's room to release this feature as an "alpha/beta/experimental" feature, i.e. might change the format a bit? I'm a bit weary of a yak shaving mission where we have to be way more sure of the format than we need to be.

Certainly. I don't want us to overdo things right away. I think that releasing something that works would be a great start and we can polish it later. Makes sense to stick to the current request/response format for the first release. Then a next step would to consider a simple mechanism to supply different responses based on the transport, but that's orthogonal to creating the transport.

shen-tian added some commits Mar 25, 2019

Removed use of keyworded-set
It's use is narrow enough that we don't need it anymore. Inlined into
clean-response

@shen-tian shen-tian force-pushed the shen-tian:edn-transport branch from d449174 to da631ef Mar 25, 2019

The edn transport is (almost) transparent. Made FnTransport cleaner
Moved the keywordizing/stringification into the bencode transport.
This might break fastlane?
@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Ok! Progress. Addressed all the comments I think, and we have some resolution on the question of the EDN protocol spec. It's the same as the internals, except I've put a shim in to make :op keywords, rather than strings. We can change the internals to actually use keywords... later. Once that's done, the transport can actually be transparent. I think this satisfies the need to get the transport out there. The shim ensures it doesn't contradict the docs, at least.

As long as we reserve the rights to adjust how the transport works later, I'm happy with that aspect for now.

In the course of this, I've moved the keywordizing/stringfication out of FnTransport. This might be a breaking change for anything that depended on it? But should be easy enough to fix at this stage.

I'll do some manual testing of the transport next..

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Everything looks great to me at this point! I'll merge this once you're done with the manual testing.

@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Just checking we are talking the same thing:

Did some manual testing per https://nrepl.org/nrepl/0.6.0/hacking_on_nrepl.html

Used variations of this code after running lein repl

(require '[nrepl.server :refer [start-server stop-server]])
(require '[nrepl.core :as nrepl])

(def server (start-server :port 7889 :transport-fn nrepl.transport/edn))
(with-open [conn (nrepl/connect :port 7889 :transport-fn nrepl.transport/edn)]
           (let [resp (-> (nrepl/client conn 1000)
                          (nrepl/message {:op :eval :code "(+ 2 2)"}))]
             (->> resp
                  (map nrepl/read-response-value)
                  nrepl/combine-responses)))

Seems to work as expected.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Yeah, that's exactly what I meant. Seems we're in business then!

I see there's some linting failure and some failing test nrepl.core-test/test-url-connect (I assume this happens randomly). Perhaps some race condition in the tests or something like this?

@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

I've made a list of tests that randomly fail above. I don't think they are EDN transport related, but rather what you see if you run through the test suite 250+ times :)

Would still like to fix those though... but later.

You are not worried about having to set the *print-readably* to make the conversion work? If not, I'm done on this one.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I had forgotten about this. Seems it's true by default, so I'm not worried about it https://clojuredocs.org/clojure.core/*print-readably*. Just add some todo in the code, and perhaps a ticket for @cichli to take a look into this problem when he has some time.

@shen-tian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

I had forgotten about this. Seems it's true by default, so I'm not worried about it https://clojuredocs.org/clojure.core/*print-readably*. Just add some todo in the code, and perhaps a ticket for @cichli to take a look into this problem when he has some time.

Left a note :)

@bbatsov bbatsov merged commit ff7fdab into nrepl:master Mar 26, 2019

11 of 12 checks passed

ci/circleci: Java 11, Clojure 1.9 Your tests failed on CircleCI
Details
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 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 shen-tian deleted the shen-tian:edn-transport branch Apr 2, 2019

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