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

Additional Disconnect Handling #255

Merged
merged 1 commit into from
Feb 27, 2024
Merged

Additional Disconnect Handling #255

merged 1 commit into from
Feb 27, 2024

Conversation

amtelekom
Copy link
Contributor

  • Allow Server Handle to trigger Disconnect
  • Allow Client Handler to read Disconnect reason

@Eugeny
Copy link
Owner

Eugeny commented Feb 26, 2024

I think the naming of Handler::disconnect is going to be confusing for the users - just today there was a user wondering how to react to session closure due to an error. Most will probably assume that it's going to be called when the session is closed for any reason - what do you think?

@Eugeny
Copy link
Owner

Eugeny commented Feb 26, 2024

Otherwise LGTM

@amtelekom
Copy link
Contributor Author

I see the problem. I would still use the word disconnect, since that's what RFC 4253 calls it.

I can see 2 Variants:

  • prefix it with received_ to hopefully make more clear we're just talking about a normal session end (22373e5)
    This now opens up the possibility to add a disconnect or connection_closed callback that is called if the main tokio::select! loop is left for any reason, or users can parse the result of the join. (

  • Pipe all kinds of disconnects through the disconnected handler (2e39628). Since the error isn't Clone and a borrow to it wouldn't be Sync, we have to move the Error through the callback in this case though.

@Eugeny
Copy link
Owner

Eugeny commented Feb 27, 2024

The 2nd variant is much clearer to the user and is universal, I like it! 👍 👍

Now just to remove this comment https://github.com/warp-tech/russh/pull/255/files#diff-a54f5b4b2c7b6955dd496e3f9b7bcc52dac6226eb3450c4785bd7957e47f0c1dR1694 and the PR is good to go

- Allow Server Handle to trigger Disconnect
- Allow Client Handler to read Disconnect reason

Rename Handler disconnect event

Include all types of disconnect in disconnected callback

Remove no longer valid comment
@amtelekom
Copy link
Contributor Author

Fixed & squashed

@Eugeny Eugeny merged commit 1d7dab8 into Eugeny:main Feb 27, 2024
4 checks passed
@Eugeny
Copy link
Owner

Eugeny commented Feb 27, 2024

Thanks - appreciate you working on these things ✌️

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.

2 participants