babashka / pods

Pods support for JVM and babashka
Eclipse Public License 1.0
123 stars 12 forks source link

More granular print/flush support in protocol #56

Closed justone closed 2 years ago

justone commented 2 years ago

I've been working to improve my bb-pod-racer library for creating Babashka Pods. The latest is providing a way for any printing (stdout) done by a pod function to be translated into Pod messages back to the parent Babashka process.

To that end, I implemented a java.io.Writer subclass called PodWriter, and I bind that to *out* when a pod function is invoked.

The problem I ran into is that the "out" message results in a println call, so extra newlines are introduced. The operations that are called in java.io.Writer are print and flush, so I tried out implementing a "print" and a "flush" message, and everything seems to work. I was able to get tabl to print a table and a test function that leveraged progrock, all over these new messages sent by the PodWriter class.

What do you think of adding the "print" and "flush" message types to the Pod protocol to support this use case?

Thank you.

borkdude commented 2 years ago

Your implementation looks good, but I'd like to take a moment to think about alternatives.

I believe "out" is modelled after nREPL but it's possible I have chosen the wrong implementing with println. I'm trying to browse the nrepl implementation to see what clients normally do for "out" ....

Another thought:

Perhaps we could also just have an "eval" op to make the client evaluate something? This way you can let the client print and flush something, without introducing specific ops... Maybe I'm overthinking this.

borkdude commented 2 years ago

I found it:

https://github.com/trptcolin/reply/blob/9d274242cf566aad0b38d36b76222d589b4c746b/src/clj/reply/eval_modes/nrepl.clj#L184-L201

I think this makes more sense for pods to do, like nREPL clients do:

Handle "out" with print followed by flush and not println.

If we change the behaviour now, we risk breaking pods, but honestly, I don't think a lot of people rely on the current behavior. The breakage would be a missing new-line, worst case.

Any thoughts here @retrogradeorbit @cap10morgan ?

justone commented 2 years ago

I just tested print followed by flush and it works perfectly. Great idea.

borkdude commented 2 years ago

PR welcome!

justone commented 2 years ago

Cool, I'll open one.

retrogradeorbit commented 2 years ago

Handle "out" with print followed by flush and not println.

I think that is a good idea. Don't rely on a new line flushing the stream.

@justone There is also the {:transport :socket} method of communicating with the pod, and this allows your pod to retain handles to stderr/stdout and the terminal. And also stdin, so (read-line) still works. So the pod can just print and it will appear on stdout. This is used in lanterna (https://github.com/babashka/pod-babashka-lanterna) and spire (https://github.com/epiccastle/spire/) and is just as fast as the stdin/stdout method (there was a recent change to make this fast).

retrogradeorbit commented 2 years ago

sorry, I see you're not trying to print, you are trying to capture print.

I don't think there needs to be any new messages. In my latest pod (bbssh) I managed to do full round trip client->pod->client and pod->client->pod without any new messages. @justone I'll have a look at your pod racer a little later see if there is anything I can suggest...

retrogradeorbit commented 2 years ago

Had a look. {:transport :socket} would be the easiest way. But you could also use pod async callbacks to make methods of a pod side class trigger client side callbacks https://github.com/babashka/pods#async . In other words, your methods of -flush and -write could trigger async flush and write callbacks on the client side (you can inject implementations of these into bb at pod describe) which would do the relevant things. But if you make the transport over a socket all the pod side stdin/stdout/stderr related functions will work as before without any changes.

borkdude commented 2 years ago

@justone I'm still keen to receive your "out" fix

justone commented 2 years ago

Cool. I’ll open a PR soon.

borkdude commented 2 years ago

What about a more general “eval” op? Then you could just send the print and flush expressions? I also wonder if out should maybe have been implemented using print vs println with automatic flush

On Sun, 25 Sep 2022 at 02:43, Nate Jones @.***> wrote:

I've been working to improve my bb-pod-racer https://github.com/justone/bb-pod-racer library for creating Babashka Pods. The latest is providing a way for any printing (stdout) done by a pod function to be translated into Pod messages back to the parent Babashka process.

To that end, I implemented a java.io.Writer subclass called PodWriter https://github.com/justone/bb-pod-racer/blob/map-out-to-bencoder/src/pod_racer/writer.clj, and I bind that to out https://github.com/justone/bb-pod-racer/blob/map-out-to-bencoder/src/pod_racer/core.clj#L107 when a pod function is invoked.

The problem I ran into is that the "out" message results in a println call https://github.com/babashka/pods/blob/master/src/babashka/pods/impl.clj#L222, so extra newlines are introduced. The operations that are called in java.io.Writer are print and flush, so I tried out implementing a "print" and a "flush" message https://github.com/justone/pods/commit/8106c648f0bc8d96ce1b2bcc8114e3e22a0c81cc, and everything seems to work. I was able to get tabl https://github.com/justone/tabl to print a table and a test function that leveraged progrock https://github.com/weavejester/progrock, all over these new messages sent by the PodWriter class.

What do you think of adding the "print" and "flush" message types to the Pod protocol to support this use case?

Thank you.

— Reply to this email directly, view it on GitHub https://github.com/babashka/pods/issues/56, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACFSBR3ZOOU5KJTF7KERCDV76N2BANCNFSM6AAAAAAQU3VDJA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- https://www.michielborkent.nl https://www.eetvoorjeleven.nu