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

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

Merged
merged 4 commits into from
Jul 3, 2023
Merged

Conversation

Ambrevar
Copy link
Contributor

@Ambrevar Ambrevar commented Jun 12, 2023

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

Fixes #4.

Needs more testing.

@Ambrevar
Copy link
Contributor Author

Ambrevar commented Jun 12, 2023

To do:

  • Synchronize client / server creation.
  • Reuse socket-stream instead of recreating it each time.

@aadcg
Copy link
Member

aadcg commented Jun 12, 2023

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

@Ambrevar
Copy link
Contributor Author

@aadcg #3 should be fixed indeed.

@Ambrevar
Copy link
Contributor Author

Ambrevar commented Jun 14, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Ambrevar commented Jun 14, 2023

To do:

  • Probably do not use alex:read-stream-content-into-string but read-line instead, otherwise evaluation happens only upon stream closure.
    Then no need to loop over connections, only need to loop over read-line. Then use iolib:with-accept-connection to close the connection and the socket when Electron is done.

@Ambrevar
Copy link
Contributor Author

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 sionescu/iolib#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
Copy link
Member

aadcg commented Jun 19, 2023

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

@Ambrevar
Copy link
Contributor Author

Should be ready to merge. The result / error separation is another issue: #10

@Ambrevar Ambrevar mentioned this pull request Jun 26, 2023
@aadcg
Copy link
Member

aadcg commented Jun 26, 2023

@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
Copy link
Member

aadcg commented Jun 26, 2023

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

Ambrevar added 4 commits July 3, 2023 11:27
This protects us from network access and possible increases network performance.
This would trigger an error with CL-JSON.
Otherwise we would not evaluate anything until the stream is closed.
@Ambrevar
Copy link
Contributor Author

Ambrevar commented Jul 3, 2023

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

@Ambrevar Ambrevar merged commit 6956b66 into master Jul 3, 2023
@Ambrevar
Copy link
Contributor Author

Ambrevar commented Jul 3, 2023

Merging since no one is opposing to it :)

@jmercouris
Copy link
Member

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

@aadcg aadcg deleted the local-sockets branch July 20, 2023 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Use local Unix socket instead of BSD socket
3 participants