search

mirage / awa-ssh

Purely functional SSH library in ocaml.
ISC License
104 stars 12 forks source link

Be able to stop the server via a `Lwt_switch.t` #52

Closed dinosaure closed 1 year ago

dinosaure commented 1 year ago

This PR adds a new argument to the Awa_mirage.spawn_server. The switch permits to the user to stop the server by a simple Net_eof signal catched by the nexus (the main loop) function. It probably closes abruptely the connection

/cc @hannesm, @reynir

palainp commented 1 year ago

Hi, does it means that a distant user will shutdown the server when closing its connexion? I hope it's deactivable, at least for my sshfs server ;) EDIT: Ah, Lwt_switch does nothing when no stop provided. So not activated by default, great for me!

dinosaure commented 1 year ago

EDIT: Ah, Lwt_switch does nothing when no stop provided. So not activated by default, great for me!

Yes, this PR adds just the ability, for the server side, to close the connection if the implementer wants to close the connection (for whatever reason) but the default behavior is the same as before - keep the connection open as long as the client wants.

reynir commented 1 year ago

At a first glance it looks good to me. I would need to study the code a bit more. Particularly, I am curious

dinosaure commented 1 year ago

if Lwt.nchoose_split keeps ordering,

I don't think that lwt assumes an order of evaluation. At least, it depends on how we implement the main loop (or the mirage-runtime in our case).

what happens when the switch is turned off? It seems to me nexus tries to send off pending outgoing packets first, at least in some cases(?)

The advantage of the Lwt_switch.t (compared to Lwt_condition.t) is that we can not miss the turned_off signals and it can appears only one time. According to the current design, an turned_off will automatically signals (and fulfill) the switched_off thread with Net_eof. We will go to this case: https://github.com/mirage/awa-ssh/blob/a9091a99e787dc4d83efbe56add678f7b1b66747/mirage/awa_mirage.ml#L265-L266

It's why I said that I probably close abruptely the connection. Indeed, some threads can still exists and they probably want to send few more bytes but I did not find a really nice way to close the connection on the server side. However, I think that we probably send Eof to channels if we turn off the switch. I will add a new commit for that.

hannesm commented 1 year ago

Thanks for your PR. To rephrase: the API extension allows an application (client of the API) to close the SSH session. Previously, any application providing a SSH server would not be able to initiate a CLOSE (but required the SSH client to close the connection).

Did I understand this correctly? Please correct me if I'm wrong.

Hi, does it means that a distant user will shutdown the server when closing its connexion? I hope it's deactivable, at least for my sshfs server ;) EDIT: Ah, Lwt_switch does nothing when no stop provided. So not activated by default, great for me!

No, there's nothing that shuts down the server. It only allows an application (such as your sshfs) to tell a client that the connection is closed, but other connection are not affected.

To me, this PR looks fine.