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

Add sideloader (see #97) #109

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@cgrand
Copy link
Contributor

cgrand commented Dec 20, 2018

It was tricky to test (as tracking responses to several messages at the same time is required)
BTW all this sequences (client code) tied to effects and control make me uneasy.

@cgrand cgrand requested a review from bbatsov Dec 20, 2018

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Dec 21, 2018

BTW all this sequences (client code) tied to effects and control make me uneasy.

Can you elaborate on this?

@cgrand

This comment has been minimized.

Copy link
Contributor Author

cgrand commented Dec 21, 2018

Writing maximally lazy lazy sequences ops has always been an art and since the introduction of chunked sequences the laziness guarantees have been relaxed. So using them for anything queue-like is perilous (eg you don't get a message because something in the middle is not as lazy (it can be as stupid as a & in destructuring where a rest would have work) so it waits for the next message before delivering the current one... but the next one won't occur until you reply or act on the current one).

It's so Clojure 1.0 (see seque, pmap etc.) :-)

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Dec 22, 2018

so it waits for the next message before delivering the current one... but the next one won't occur until you reply or act on the current one

So, the main concern is whether the provided things are going to be loaded in the right order without being serialized, right?

Btw, it's a bit hard for me to figure out the exact message exchange that has to happen between the client and the server here. Can you write down some pseudo-code message exchange?

I assume the client does sideloader-start and then sends a bunch of sideloader-provides, but I couldn't infer this from the tests - I just remember our conversation on the topic. :-)

@cgrand

This comment has been minimized.

Copy link
Contributor Author

cgrand commented Dec 22, 2018

No, the main concern is that in my experience using sequences as queues ia a fragile equilibrium. It can be made to work but is very brittle. In the present case it could be possible to have a deadlock purely because seq processing introduced an accidental lag.

I definitely have to document the side loader more.
Everything occurs in a single session.
You first send a sideloader-start message to, well, start the sideloading mechanism.
This message never gets a "done" response. It only gets a stream of sideloader-lookup responses (as required by evaluation).
In reaction to these sideloader-lookup the client MUST send a new message (different uuid) sideloader-provide possibly with empty content (when the resource is not known to the client). The server will reply with done.

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Dec 28, 2018

Makes sense. Thanks for the explanations!

Btw, how can we make this work with middleware - one problem that I see is that currently you have to start the server with right handler (a handler using all the middleware needed by the client) before connecting to the server. The sideloading would handle nice the case where you just need more libraries to eval code from, but I'm not certain how we can approach the middleware problem (and potentially transports as well).

@cgrand

This comment has been minimized.

Copy link
Contributor Author

cgrand commented Jan 7, 2019

@bbatsov for transports, do you have an actual scenario in mind?

For middlewares I can see two scenarios :

  1. Setting up the stack.
  2. Adding to the stack.

The first case should be covered by injection. My idea for injection is to first send a bootstrap payload which turns the current socket repl in something which resembles (from a wire perspective) to EDN (or bencode but hacky) transport + side loader. This payload puts the socket repl in a state where it's basically evaluating the equivalent of start-server so it's going to request all classes and clj files as needed.

(with-bootstrap-sideloader ; pseudo code as it could me more verbose to be deps free
  (require 'nrepl.server)
  (nrepl.server/upgrade (read)))
[additional middlewares] ; read by the read call above
@arichiardi

This comment has been minimized.

Copy link
Contributor

arichiardi commented Jan 7, 2019

You first send a sideloader-start message to, well, start the sideloading mechanism.
This message never gets a "done" response. It only gets a stream of sideloader-lookup responses (as required by evaluation).
In reaction to these sideloader-lookup the client MUST send a new message (different uuid) sideloader-provide possibly with empty content (when the resource is not known to the client). The server will reply with done.

Nice and concise explanation!

I am sure this is probably not true, but I think I have a nice name for the message from server to client requesting a dependency: sideloader-requirement xyz! To which the client responds sideloader-provide xyz 😄

Yeah a bit long 😄 Sorry for abruptly barging in but I am your best fan (...creepy...) 😄 👍

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 7, 2019

@bbatsov for transports, do you have an actual scenario in mind?

Mostly to the first one. I was a bit silly and forgot that we'd deliver with the initial injection, not with the sideloader.

  1. Adding to the stack.

That might be nice to tackle at some point, but it's certainly not as important as 1.

@cgrand

This comment has been minimized.

Copy link
Contributor Author

cgrand commented Jan 8, 2019

@arichiardi well I don't feel that requirement is a good match for the semantics (it's too strong imo). What about changing sideloader-provide to sideloader-found?

@arichiardi

This comment has been minimized.

Copy link
Contributor

arichiardi commented Jan 8, 2019

@cgrand I found lookup not too descriptive but you are the master so I am not going to debate more 😄 I like found as well 😉

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