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

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

Merged
merged 4 commits into from
Mar 2, 2023

Conversation

dinosaure
Copy link
Member

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

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.
@dinosaure dinosaure changed the title Be able to stop the server via an Lwt_switch.t Be able to stop the server via a Lwt_switch.t Feb 28, 2023
@palainp
Copy link
Member

palainp commented Feb 28, 2023

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

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

reynir commented Mar 1, 2023

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

  • if Lwt.nchoose_split keeps ordering,
  • 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(?)

@dinosaure
Copy link
Member Author

dinosaure commented Mar 1, 2023

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:

(* Here we have the net_read tells us to stop the communication... *)
| Net_eof :: _ -> Lwt.return t

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

hannesm commented Mar 1, 2023

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.

mirage/awa_mirage.ml Outdated Show resolved Hide resolved
@dinosaure dinosaure merged commit 7608e31 into mirage:main Mar 2, 2023
@dinosaure dinosaure deleted the stop-server branch March 2, 2023 13:15
hannesm added a commit to hannesm/opam-repository that referenced this pull request Mar 22, 2023
CHANGES:

* server: be able to stop using a Lwt_switch.t (mirage/awa-ssh#52 @dinosaure)
* server: add Pty/Set_env/Start_shell events (mirage/awa-ssh#53 @dinosaure)
* client: support password authentication and keyboard-interactive (mirage/awa-ssh#51
  @hannesm, reported by @dgjustice mirage/awa-ssh#31)
* client: add NIST EC curves (mirage/awa-ssh#31 @hannesm)
* client: try public key authenticaion only once (mirage/awa-ssh#50 @reynir @hannesm)
* remove (partially implemented) hostbased authentication (mirage/awa-ssh#31 @hannesm)
* replace deprecated Cstruct.copy by Cstruct.to_string (mirage/awa-ssh#53 @dinosaure)
* remove ppx_cstruct and sexplib dependencies (mirage/awa-ssh#54 @hannesm)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants