Skip to content
This repository has been archived by the owner on Jun 4, 2022. It is now read-only.

Suspension #334

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Suspension #334

wants to merge 9 commits into from

Conversation

cgrand
Copy link

@cgrand cgrand commented Dec 21, 2017

This is a solution to #294.

User code wanting to take control of the repl must evaluate to a suspension request (created with lumo.repl/suspension-request).

When the repl sees a suspension request being returned, instead of printing it and "looping", it suspends itself and yield control to the callback embedded in the suspension request.

Two arguments are passed to this callback: an object satisfying lumo.repl/AsyncReader (the input stream) and a resume callback to call when user code reaches completion to resume the repl. This callback take a map with keys :value, :error and :ns. The repl resumes at the "print" phase.

The AsyncReader protocol has been introduced as a platform agnostic interface over streams -- even if it's currently namespaced in lumo.

Copy link
Collaborator

@arichiardi arichiardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is worth a couple of tests I think, just to understand how the flow looks like, in general it looks good!

spill! #(loop []
(when-some [s (.pop back)]
(do (.push front s) (recur))))]
#js [(fn
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is probably worth giving a name to fn just to make the pattern clearer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this loop [] is useless it should be

+        spill! #(when-some [s (.pop back)]
+                  (do (.push front s) (recur)))]

where would you like a name?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the fn inside the Javascript array

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, it's just #(when-some [s (.pop back)] (.push front s) (recur)) ...

back #js []
cb (volatile! nil)
spill! #(when-some [s (.pop back)] (.push front s) (recur))]
#js [(fn
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean this line:

(fn 
        ([]
         (spill!)

The two-arity we, minor anyways

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments to each arity in 8f8eec4

@cgrand
Copy link
Author

cgrand commented Dec 22, 2017

circle failure is due to Node version (change in an error message)

@arichiardi
Copy link
Collaborator

I really really like this patch.

One thing I wanted to add though, kind of tangential, is a question: how likely a change like this is going to end up in clojure/clojurescript?

Because the unrepl protocol is really cool but this patch requires a big buy-in that might not be accepted over there. I know I don't know the answer I am not the maintainer and I know lumo can differ from cljs, I just want to trigger some conversation around this.

At the beginning I thought the unrepl protocol could be implemented on top of the socket accept function. If it still can @cgrand, that would be less invasive and would not need any update on either lumo or cljs (hopefully).
If this same patch could be applied to the receive socket function IMHO it would be awesome.

@cgrand
Copy link
Author

cgrand commented Jan 8, 2018

@arichiardi accept in -n makes nothing that wasn't possible with -m possible, it just standardizes how to set host and port.

The current patch changes what is possible with a lumo REPL by making a call to "read" (or generally access to the input stream) from user code possible as it is in many lisps.

It happens that's something leveraged by unrepl but it's of broader use.

At the conj this year @anmonteiro asked me to find the smallest change that could make lumo upgradable (because my previous attempts were too large: a new reader, a new main loop and a complected IO iface), this is my best answer so far.

@arichiardi
Copy link
Collaborator

Ok great then, I was sure you had considered this already, just to make it clear, I would talking about to apply the save suspension mechanisms to the acccept-fn here: https://github.com/futuro/lumo/blob/7709e3a772b4767c5fa1fda2580187538573f120/src/cljs/snapshot/lumo/repl.cljs#L1317

In any case, I don't feel confident merging this myself so I'll let Antonio have a look.

@arichiardi
Copy link
Collaborator

So @cgrand kindly explained me that I was completely off. The suspension happens before the REPL invokes the accept function and therefore sorry for the noise and the misunderstanding 😄

@cgrand
Copy link
Author

cgrand commented Jan 9, 2018

@arichiardi I guess I haven't been that successful in conveying the idea across: when you provide an accept function there's no REPL, the Lumo REPL is never started on the socket (the accept function may happen to be a REPL but it can be anything).
The suspension mechanism I introduce is part of the Lumo REPL so is accessible only when the Lumo REPL is running, hence not when an accept function is provided.

@cgrand
Copy link
Author

cgrand commented Feb 24, 2018

@arichiardi could you reopen this PR has it was closed by a typo on the issue number?

@arichiardi arichiardi reopened this Feb 24, 2018
@cgrand
Copy link
Author

cgrand commented Feb 24, 2018

@arichiardi thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants