search

atlas-engineer / cl-electron

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

core(launch): Simplify existing socket logic. #38

Closed aadcg closed 7 months ago

aadcg commented 7 months ago

Commit adba6d2009d76acbeacf013cd3370f5c50b355e7 introduced a restart for the case when the socket already exists, but I don't think this is a good idea. Firstly, it harms the tests since a condition is raised on each round-trip of with-electron-session. Secondly, under which circumstances would we want not to delete a dangling socket? Only when there would be multiple Electron processes launched by launch... But they would be sharing the same socket, which doesn't seem correct. Before, there was the guarantee that only one Electron process could be spawned. Now, there's no such limitation. Why would we want to pursue this strategy? Besides the commit I've pushed to this PR, I'd suggest getting rid of the ignore restart and launch-process unless alive-p (as before), as to guarantee that only one process is spawned.

As an alternative to the approach described above, (uiop:delete-file-if-exists (electron-socket-path interface)) should run on terminate.

On a tangent, (1) this change was orthogonal to PR #34; (2) a proper review should have taken place; (3) the tests weren't probably run before merging #34.

jmercouris commented 7 months ago

I should have separated things out into separate PRs. I did run the tests initially, but then didn't run them on subsequent changes that were orthogonal.

jmercouris commented 7 months ago

We cannot guarantee that terminate will run. I added this restart to handle cases for when cl-electron crashes and doesn't exit cleanly.

With regards to always deleting the socket, we can do that. However, there is a loss of information in this scenario in which we do not signal a condition. So, we wouldn't know if a previous instance of cl-electron had crashed, or what exactly had happened.

jmercouris commented 7 months ago

Whether we actually need to know that a previous instance of cl-electron crashed may or may not be relevant.

Summary: I am OK with this change, but it results in a loss of information.

aadcg commented 7 months ago

@jmercouris, based on your input, I have arrived at the current suggestion. Does it have any shortcomings?

jmercouris commented 7 months ago

I have no problem with the revised change. Only comment, two cases, should use an if rather than a cond, I think it is clearer.

aadcg commented 7 months ago

Only comment, two cases, should use an if rather than a cond, I think it is clearer.

When a prog needs to be involved, I think both are equally OK.