-
Notifications
You must be signed in to change notification settings - Fork 1
Add spec #1
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
base: main
Are you sure you want to change the base?
Add spec #1
Conversation
Shouldn't close the socket on close op because one socket might have multiple sessions. Explicitly document multiple sessions on one socket.
While I agree it's not a must, I can speak a bit to its usefulness - for me the main value is that you can split "user evaluations" (the code the user actually wanted to evaluate) from "tooling evals" (e.g. using One other practical issue that I see without creating sessions explicitly is that you'd have a harder time mapping |
| `clone`, `eval`, `stdin`, and `describe`. Clients may support | ||
| `describe` but this is not required. | ||
|
|
||
| ### `clone` op |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to learn from the doc why this op is called clone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because it indeed "clones" the session (specifically, the state of all its dynamic variables) specified in the clone request. When you "clone" an "ephemeral" (unspecified) session, that doesn't matter because there is no divergent state yet. However, you can also "clone" an existing session and inherit all of its state.
Whether this is useful to any existing client is another story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@technomancy I may argue that sessions and clone are not fundamentals of the nREPL protocol in the widest sense.
Sessions provide three specific benefits:
- Retention of thread-local state. That comes naturally for any Java thread-bound facility (like ThreadLocal) by reusing the Thread for handling
evalrequests; but also ensures dynvar state preservation in case anyevalisinterrupted (in that case, the thread dies but session lives on and all dynamic variable bindings remain intact). - Serialization of
evalrequests. From the user's perspective, it is desirable that hittingC-x C-etwice will execute evaluations one after another, not concurrently, as if the user typed those two forms into the REPL. It is easier and more reliable to implement this on the server side than to trust client to not issue simultaneous evals. So, stable "sessions" offer such serialization in an otherwise asynchronous protocol. - Interruption is currently tied to sessions in Clojure nREPL, but that is an implementation detail, it doesn't have be.
Otherwise, sessions are not required for nREPL to operate. The client can issue eval requests without a session identifier (we call those "ephemeral sessions" inside). In languages that don't have thread-local state, sessions might not be necessary. Request serialization might also not be necessary.
However, if we say that request serialization and thread-local state retention are necessary for any nREPL server to implement, the spec must then clearly define what this entails. Because, currently, Clojure nREPL has implemented it very stochastically. For example, eval, load-file, CIDER's test runner will all serialize while the other "tooling" operations (completion, lookup, etc.) will work concurrently. If sessions are part of the spec, the server implementor has to be informed what implementing a session truly means besides just returning a random UUID on clone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. The main point of my comment was that some explanation (which may be part of what you responded with) could be added to the doc for readers new to the topic. Same applies to my other comment.
|
|
||
| ``` | ||
|
|
||
| ### `describe` op |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is reasonable to demand on the spec level that all compliant clients start their interaction with the server by issuing a describe command.
- It acts as a good sanity check that they've connected to a proper nREPL server, with the correct transport and encoding. The server doesn't issue a proactive greeting/green light on connecting to it.
- In the absence of an official per-connection "init" request, it works as a vessel to pass initial data about the client to the server (we recently started using it for recording client versions for the connection).
- To promote maximum client-server compatibility, clients must only attempt issuing requests for ops that are supported by the server, and not assume anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, yes! Using describe as an entry point is a good sanity check. Perhaps we could combine it with the session registration; like if we're OK with diverging more from the existing patterns we could make a register op which returns the results that describe currently and also registers a session.
I agree that encodings and transports aren't good examples here; I'll get rid of them. Open to suggestions for what other fields would make more sense to demonstrate. Or maybe there isn't a need to express extensions that aren't ops?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before addressing this suggestion, I'll wait for you to respond in another thread about sessions.
| "id": "5d90576e-b5e1-4499-a43d-c75c60b579ff", | ||
| "ops": ["clone", "eval", "stdin", "describe", "load-file", "sandbox"], | ||
| "features": {"encodings": ["bencode", "json"], | ||
| "transports": ["socket", "stdio"]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is just an example, but none of this data is really actionable for the client. Knowing that a server supports JSON encoding is quite useless if you don't know the port where to connect to it. So, I'm not sure how I would use this features map in a way that provides something more over ops, but is sufficient in this suggested shape.
| receives a request with an `op` it does not recognize, it must reply | ||
| with a message whose `status` contains `unknown-op` along with `done`. | ||
|
|
||
| ### `interrupt` op |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we are entering the twilight zone here. Can we really postulate in the spec how optional extensions should be implemented? What if they do it differently, does it make the implementor non-compliant to the spec? Or is it obligatory for the servers to implement interrupt but optional for the clients to use it? Same goes for lookup and completions mentioned below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good question. The intent is to say "look, if you are interested in doing more than just eval, here's how to do it" because the alternative is that clients end up sending code across the wire; that's the main thing we are trying to avoid.
We can't make interrupt a required op, because it's literally impossible for many servers to implement it. But interrupt is a special case so maybe it's not the best example.
I definitely want to allow implementers to support a bare minimum and not worry about fancy features. One alternative would be to have a tiered approach. A tier 1 server implements everything in the spec (except perhaps interrupt if the runtime doesn't support it) while a tier 2 server implements just clone, eval, stdin and describe.
But maybe this is overcomplicating things. We should look at how LSP structures their operations, because I know that they have an awful lot that are extra fancy and not widely supported. Maybe it's fine to just toss everything in the spec and let implementers choose which ones they want? Clearly some ops are more important than others tho...
| containing a path to the archive file. If `archive` is present, then | ||
| `file` is interpreted as being inside the archive; otherwise it is | ||
| either an absolute path or interpreted as being relative to the | ||
| directory in which the server was started. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this feels like the spec declaring how it would like for an extension to operate without any real authority since this is all optional. I think it should either be required or completely left out for the implementors to decide.
|
|
||
| If the `sym` is not found, then the `info` field should be omitted. | ||
|
|
||
| ### `load-file` op |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a heavyweight change from how load-file works now, I would say compatibility-breaking. The current load-file pastes the code to load verbatim, and thus is not much different than eval (and we may soon replace all usages of load-file in CIDER with calls to eval). The load-file described here is a different beast, and we have to think more before introducing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to support the old style and the new style at the same time; the old style required a file parameter. So if you want to retain compatibility, you do the old behavior when that is present and do the new behavior otherwise.
But yes, putting the old behavior in the spec makes no sense; it should never have been added. Note that the new behavior is optional; if clients want to continue just using eval that's fine.
@bbatsov this feels like it's only relevant if you're sending things that should be ops as Also, to clarify the impact is solely that of mixing up
I think it's actually easier to associate messages with the socket that they came in on, rather than creating a completely separate session abstraction. |
| {"session": "afd3c88e-707f-4169-a265-892f29476333", | ||
| "id": "5d90576e-b5e1-4499-a43d-c75c60b579ff", | ||
| "ops": ["clone", "eval", "stdin", "describe", "load-file", "sandbox"], | ||
| "features": {"encodings": ["bencode", "json"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supposedly, we need to know which encodings the server supports to even send this describe op, right? How one is supposed to use this feature?
Also, does features and versions have some specific format, or can I use this to offer additional stuff (like, for example, the language that this nREPL is supposed to be evaluating)?
Adding the spec.
I've made these four ops required:
These are the optional ones:
There are a few minor incompatibilities with the old protocol, both noted:
load-filenow loads a file, and theopsin thedescribeop is now a list instead of a dictionary with unspecified values.I've added specificity to
lookupso that it explains how to use it for docstrings, arglists, and definition location. The way that definition locations use a separatearchivefield is I think different from the current behavior in Clojure (I think it just mushes the archive and the path inside the archive all into one field right now?) but a lot more convenient for client authors.It's currently specified that servers should support multiple sessions on a single socket. I don't think this is particularly useful, but once you have session handling it's also not particularly difficult either, so I've left it in. Maybe I'm just missing something here and this actually is quite useful? TBH the
cloneop seems unnecessary; it should be possible to immediately make anevalordescriberequest and get anew-sessionin the response, or just say that every message on a socket is assumed to be part of the same session. I don't want to do a Chesterton's Fence here, so I could use some input on what purpose this was meant to serve. Maybe @cemerick could chime in here.I've also introduced a free-form
featuresdictionary to thedescriberesponse as a place explicitly intended for extensions. Hopefully this will encourage implementers to do things more declaratively and collaborate on a standard way to do things instead of reinventing the wheel.