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

Implement shutdown, as required by mirage-flow 4.0.0 #512

Merged
merged 5 commits into from
Mar 26, 2024

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Dec 21, 2023

Would be great to have a review on this.

close

From my understanding, in the UNIX sockets API the SO_LINGER is relevant (to specify how long close() may block) - since we don't have SO_LINGER, let's just care about the normal case (text paraphrased from https://www.cl.cam.ac.uk/~pes20/Netsem/alldoc.pdf):

  • close sends any remaining data and gracefully closes the connection
  • if close is called on a pre-established state: socket is closed and removed
  • if on a listening socket: close and remove that socket, and aborts each of the connections on the socket's pending and completed connection queues.

If I understand, our flow is never a listening socket. Our unlisten eventually should care about the last point.

shutdown

Depending on `read | `write | `read_write, the specific half is shut down:

  • read: shutting down the read-half empties the socket’s receive queue, but data will still be delivered to it and subsequent recv() calls will return data.
  • write: Shutting down the write-half of a TCP connection causes the remaining data in the socket’s send queue to be sent and then TCP’s connection termination to occur.

TL;DR

There's some discrepancy from my intuition (and the man page shutdown(2) SHUT_RD and the model: according to the man page) "Further receives will be disallowed". In this PR, I push None to the Lwt_dllist -- with the hope that this leads to the expected behaviour (those who have knowledge about the internals should comment whether there's something else that needs to be done).

Copy link
Member

@djs55 djs55 left a comment

Choose a reason for hiding this comment

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

LGTM. I'm not sure exactly how the SHUTDOWN_RECEIVE is supposed to behave but I've never found a use for it. The SHUTDOWN_SEND to perform a half-close is really useful though and this looks good.

@hannesm
Copy link
Member Author

hannesm commented Mar 26, 2024

Thanks @djs55. I'll go ahead with merge and release once CI is green.

@hannesm hannesm merged commit 7a2e6a7 into mirage:main Mar 26, 2024
3 checks passed
@hannesm hannesm deleted the shutdown branch March 26, 2024 14:48
hannesm added a commit to hannesm/opam-repository that referenced this pull request Mar 26, 2024
CHANGES:

* TCP: add `src : flow -> ipaddr * int`, implemented by `getsockname` on unix
  (mirage/mirage-tcpip#511 @hannesm)
* TCP unix stack: increase TCP buffer size (was 4096, is now 65536)
  (mirage/mirage-tcpip#510 @edwintorok)
* TCP: adapt to mirage-flow 4.0:
  add ``val shutdown : flow -> [ `read | `write | `read_write ] -> unit Lwt.t``
  (mirage/mirage-tcpip#512 @hannesm, review by @djs55)
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