Skip to content

Fixed [msg_channel_data] buffering and size issues.#78

Merged
reynir merged 4 commits intomirage:mainfrom
gravicappa:fix-outgoing-data
Apr 10, 2025
Merged

Fixed [msg_channel_data] buffering and size issues.#78
reynir merged 4 commits intomirage:mainfrom
gravicappa:fix-outgoing-data

Conversation

@gravicappa
Copy link
Contributor

Before [msg_channel_eof] was sent without draining [channel.tosent] buffer. Also sending large amount of data using [msg_channel_data] was not possible since no [msg_channel_window_adjust] was issued.

Now [Client.eof] flushes channel buffer. And [Client.outgoing_data] issues [msg_channel_window_adjust] before sending data.

Before [msg_channel_eof] was sent without draining [channel.tosent]
buffer. Also sending large amount of data using [msg_channel_data] was
not possible since no [msg_channel_window_adjust] was issued.

Now [Client.eof] flushes channel buffer. And [Client.outgoing_data]
issues [msg_channel_window_adjust] before sending data.
@gravicappa
Copy link
Contributor Author

PR addresses #77 issue.

@reynir reynir self-requested a review April 10, 2025 07:57
Copy link
Member

@reynir reynir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think you made some good and important observations - that we should try to flush pending data before sending EOF. FWIW I think this is also an issue in the server code.

@hannesm
Copy link
Member

hannesm commented Apr 10, 2025

@reynir said:

FWIW I think this is also an issue in the server code.

I agree, but in the lib/server.ml we don't have a eof function or similar... so we are never able to close a channel. I guess we can revise at some point the server code to handle channel shutdown properly -- but I wouldn't impose this on this PR.

@hannesm
Copy link
Member

hannesm commented Apr 10, 2025

From my point of view, this is ready to merge. Thanks a lot. @reynir please merge if you're satisfied.

Copy link
Member

@reynir reynir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@reynir reynir merged commit 422f7d6 into mirage:main Apr 10, 2025
1 check passed
hannesm added a commit to hannesm/opam-repository that referenced this pull request Apr 24, 2025
CHANGES:

* Drain channel.tosent before sending msg_channel_eof (mirage/awa-ssh#78 @gravicappa)
* Server: delay authentication to effectful layer - patches from banawa
  (mirage/awa-ssh#74 @reynir)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants