atlas-engineer / cl-electron

Lisp Interface to Electron.
BSD 3-Clause "New" or "Revised" License
16 stars 1 forks source link

WIP: Switch from BSD sockets to Unix sockets. #7

Closed Ambrevar closed 1 year ago

Ambrevar commented 1 year ago

This protects us from network access and possible increases network performance.

Fixes #4.

Needs more testing.

Ambrevar commented 1 year ago

To do:

aadcg commented 1 year ago

A good test would be to see if it would fix #3.

Ambrevar commented 1 year ago

@aadcg #3 should be fixed indeed.

Ambrevar commented 1 year ago

Unix sockets are bidirectional, so there is no need for having two of them here. Actually, server.js already write the result / error to the socket, which is then returned by send-message, but it seems that nothing is done with that.

Instead, the client socket is written to in register-before-input-event, but it's not very clear what for.

@jmercouris Could you shed some light on the socket architecture?

EDIT: Answer to myself: I suppose this client socket is used for anything non-functional, so that Electron can execute arbitrary code on the Lisp before returning.

Ambrevar commented 1 year ago

We also need a way to distinguish between results and errors. I suppose that sending

(:result ${result})\n

and

(:error ${result})\n

should do the trick.

Ambrevar commented 1 year ago

To do:

Ambrevar commented 1 year ago

There is a bit of a problem: IOLib seems to have issues ping-ponging read calls between client and server of a Unix socket; see https://github.com/sionescu/iolib/issues/79.

Any idea what to do? @jmercouris @aadcg @aartaka ?

Possible workaround: Use read-line instead and collect lines until uiop:safe-read-from-string does not fail on the collected stream.

aadcg commented 1 year ago

Interesting. Let's wait for the author's feedback, which tends to be fast and insightful.

Ambrevar commented 1 year ago

Should be ready to merge. The result / error separation is another issue: https://github.com/atlas-engineer/cl-electron/issues/10

aadcg commented 1 year ago

@Ambrevar I've pushed 5f94ffb. Feel free to squash it onto your commits.

I've also updated the top post since this PR also closes #3.

Look ready to be merged to me! Thanks.


I don't know how you've set up your dev environment but you may find the following useful. Add the snippet below to sly-lisp-implementations.

(cl-electron
 ("guix" "shell" "-D" "-f" "/absolute/path/to/cl-electron/guix.scm"
  "--" "sbcl"))
aadcg commented 1 year ago

It seems that you have set #11 to close #3. I'll remove the entry in the top post.

Ambrevar commented 1 year ago

I've actually removed the dep on Alexandria and Trivial-package-local-nicknames for now.

Ambrevar commented 1 year ago

Merging since no one is opposing to it :)

jmercouris commented 1 year ago

Thank you Pierre! I was going to merge this myself, but wasn't sure if you wanted to do it!