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

[2.0] Printing from go-loop not working in REPL window #218

Closed
bpringe opened this issue Jun 10, 2019 · 16 comments
Closed

[2.0] Printing from go-loop not working in REPL window #218

bpringe opened this issue Jun 10, 2019 · 16 comments
Labels
Projects

Comments

@bpringe
Copy link
Contributor

@bpringe bpringe commented Jun 10, 2019

I realized this issue when running a larger system with multiple go-loops. I saw printing from maybe one of multiple go-loops, so I don't know that it never works, but I have found how to reproduce it with a small example from clojuredocs.org. Link for reference: https://clojuredocs.org/clojure.core.async/go-loop#example-542c8833e4b05f4d257a297a

image

As you can see, nothing is printing, as it shows it should be in the clojuredocs example. I checked the terminal that the REPL is running from as well, and nothing was there either.

FYI, I'm using Windows 10 and cmd as the shell. I don't know if that matters.

Also, this is what happens if I run the code in a repl straight in cmd (also same in powershell). When I hit enter/return I can see the print statements bundled together.
cmd-go-loop-printing

My coworker confirmed that this printing works correctly in emacs/cider:
image

@PEZ

This comment has been minimized.

Copy link
Collaborator

@PEZ PEZ commented Jun 10, 2019

OK. So I know what is going on. It's here:

https://github.com/BetterThanTomorrow/calva/blob/master/calva/nrepl/index.ts#L224

When the repl client gets a state: ["done"] message, it resolves the promise and returns true and subsequent out messages no longer have anyone listening to them.

Not sure how that should be handled... If I return false instead I get the prints, but I will have a lot of promises hanging around if I do that.

@bbatsov , do you have any advice for me here?

@bbatsov

This comment has been minimized.

Copy link
Contributor

@bbatsov bbatsov commented Jun 11, 2019

That's a tricky one and in CIDER I solve it by just mapping a callback to the id of the request, so responses are properly handled even after "done" is received. Basically there's a mapping between each request and some lambda that gets called to process responses with that request id. Probably not the nicest design, but it gets the job done.

Alternatively you can just have a generic callback for out/err messages that is used when some message arrives either without an id or it has been fully processed already (you've received "done").

@PEZ

This comment has been minimized.

Copy link
Collaborator

@PEZ PEZ commented Jun 11, 2019

Thanks. I guess the promise is such a mapping. Hmmm...

I was thinking about doing the generic callback thing, but got stuck on how to allow the request to be interrupted.

@bbatsov

This comment has been minimized.

Copy link
Contributor

@bbatsov bbatsov commented Jun 11, 2019

I just keep a list of all "pending" eval requests (ones who haven't received done) and if an interrupt is sent I loop over all of them. Requests are processed sequentially within each nREPL session, but we can just try to kill the last one that was sent, as the users might have queued up several requests for evaluation (even though this rarely happens in practice).

@bpringe

This comment has been minimized.

Copy link
Contributor Author

@bpringe bpringe commented Aug 20, 2019

I just want to check in and see if any progress has been made here, @PEZ . I went back to using the legacy Calva a while back since I needed this functionality for my daily work (I work with systems that spin up several "processes" for data pipelines and need to see logs as things flow through).

I'm hoping the legacy Calva is not discontinued before this functionality is added to 2.0.

As always, thanks for all the work you've been doing. I can't wait to get back to the new version, and perhaps I can try to tackle this again. I briefly looked at it in the past and it was a bit over my head. It would require a good bit of digging in on my part but maybe I can swing the time eventually.

@PEZ

This comment has been minimized.

Copy link
Collaborator

@PEZ PEZ commented Aug 21, 2019

Hello again,

No progress here in the code, but there is progress in my understanding of the problem and I think I am about to figure out how I want to solve it.

Also, @kstehn has been looking some into streaming responses and it has taught us some more about the whole thing with our message handlers.

As for legacy Calva. It has already been discontinued. There was a last call for objections some weeks ago, #267, and nothing turned up that I deemed serious enough to warrant the confusion and extra maintenance.

Meanwhile, until we've sorted this issue, instead of using that old build, maybe you can use a workaround. Fire up a terminal and type lein repl :connect to get a REPL prompt. That will let you monitor output the same way as with legacy Calva.

@PEZ PEZ added this to Backlog in CT 2019 Q3 via automation Aug 21, 2019
@PEZ PEZ moved this from Backlog to Triaged in CT 2019 Q3 Aug 21, 2019
@bpringe

This comment has been minimized.

Copy link
Contributor Author

@bpringe bpringe commented Aug 21, 2019

Glad to hear there is some mental progress. Thanks for the workaround!

@bpringe

This comment has been minimized.

Copy link
Contributor Author

@bpringe bpringe commented Aug 21, 2019

@PEZ The workaround is not showing me all the logs =/. I think I will go back to using a custom build for now, where I return false always when the status is "done" as you said before, which worked for me in the past (though I know this is not a good solution because of promises hanging around).

It's either that or I switch to another editor, and I'd rather spend my time getting work done than learning another editor 😄 .

@PEZ

This comment has been minimized.

Copy link
Collaborator

@PEZ PEZ commented Aug 21, 2019

The workaround really should show you any logs that Calva Legacy did, because lien :connect is what Calva Legacy did...

@bpringe

This comment has been minimized.

Copy link
Contributor Author

@bpringe bpringe commented Aug 21, 2019

Hmm yeah I'm not sure what exactly is going on. To be clear though, the logs I do see are the logs I used to see in the initial repl terminal that I started myself (those are not all the logs). When I would connect to that repl session in legacy Calva, it would open a new repl terminal titled "Clojure REPL" or something similar, that connected to the original session. So then I have two repl terminals open to the same nrepl session. The logs I am not seeing now would show up in the newly created Calva "Clojure REPL" terminal, and the logs I am seeing now would show up in the original terminal where the repl session was started.

It's a bit hard to explain without diving into my own project's details.

@bpringe

This comment has been minimized.

Copy link
Contributor Author

@bpringe bpringe commented Aug 22, 2019

So I've been playing around and I noticed something maybe worth noting here. With the go-loop example from the top, if I change the print to println, the print statements show up in the repl as they occur. Also when I used taoensso.timbre/debug to print, it also worked as desired. However, I don't know why in my coworker's example from emacs/cider, the print statement appears to work like println.

So basically I'm wondering if this is no longer an issue somehow... Testing with my system and will report back.

@PEZ

This comment has been minimized.

Copy link
Collaborator

@PEZ PEZ commented Aug 22, 2019

I think the difference could be that CIDER handles streaming response and Calva does not. (Guessing here, maybe @kstehn has some more info.)

@kstehn

This comment has been minimized.

Copy link
Contributor

@kstehn kstehn commented Aug 22, 2019

Actually calva does this already.
To get multiple responses to the messageHandler, the hnadler just needs to return false.
As long as this happens the handler stays in the handler list.
In my fork you can see how that needs to be used. link

Regarding the stream prints. I'am not sure how those responses look like but i would just print them out in the repl Window. And depending what Sessiontype (clj/cljs) choosing the CLJ-Window or CLJS-Window. Nothing else is happing in the normal repl window.

@PEZ

This comment has been minimized.

Copy link
Collaborator

@PEZ PEZ commented Aug 22, 2019

@bpringe

This comment has been minimized.

Copy link
Contributor Author

@bpringe bpringe commented Aug 22, 2019

So I've been playing around and I noticed something maybe worth noting here. With the go-loop example from the top, if I change the print to println, the print statements show up in the repl as they occur. Also when I used taoensso.timbre/debug to print, it also worked as desired. However, I don't know why in my coworker's example from emacs/cider, the print statement appears to work like println.

So basically I'm wondering if this is no longer an issue somehow... Testing with my system and will report back.

Ignore this, somewhat. println logging and I think also timbre logging were only working because my custom extension changes (returning false instead of true) were still in effect. In the current version of Calva, the output does not show, as discussed before.

kstehn pushed a commit that referenced this issue Sep 21, 2019
Addressing #218
@PEZ

This comment has been minimized.

Copy link
Collaborator

@PEZ PEZ commented Sep 29, 2019

Fixed by #334

@PEZ PEZ closed this Sep 29, 2019
@PEZ PEZ moved this from Triaged to Done in CT 2019 Q3 Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
CT 2019 Q3
  
Done
4 participants
You can’t perform that action at this time.