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

Add setsockopt() to eio sockets #365

Closed
wants to merge 3 commits into from

Conversation

bikallem
Copy link
Contributor

@bikallem bikallem commented Nov 9, 2022

This PR implements setsockopt() function to Eio.Net.socket.

Fixes #322

Copy link
Contributor

@haesbaert haesbaert left a comment

Choose a reason for hiding this comment

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

I can't reason much about why changing the type declaration to classes, it seems to be a change for the sake of change ? Or what am I missing. If so, it would make more sense to put those on a different commit to make reviewer easier.

Since you're providing setsockopt, should we provide getsockopt as well ?

lib_eio/unix/eio_unix.ml Outdated Show resolved Hide resolved
lib_eio_linux/eio_linux.ml Show resolved Hide resolved
lib_eio_linux/eio_linux.ml Show resolved Hide resolved
@bikallem bikallem force-pushed the setsockopt branch 2 times, most recently from ff67de2 to 3b5a8c2 Compare November 30, 2022 10:30
@bikallem
Copy link
Contributor Author

Since you're providing setsockopt, should we provide getsockopt as well ?

Indeed, I was thinking along the same lines but wanted to get this PR reviewed and merged first. I have opened issue #376 for this.

@bikallem bikallem force-pushed the setsockopt branch 2 times, most recently from 920912c to 8e565b0 Compare November 30, 2022 11:11
Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

It's not actually necessary to create a separate Mock.Net.stream_socket type (which is an API change). We could just have Flow.t implement the socket methods too. It already works as both a source and a sink, for example.

We could just add the new method to the implementations in eio_linux, etc, rather than using inheritance, to shrink the PR. Though this seems fine to me either way.

lib_eio/mock/net.ml Outdated Show resolved Hide resolved
lib_eio/net.mli Outdated Show resolved Hide resolved
@bikallem bikallem force-pushed the setsockopt branch 4 times, most recently from cf51b49 to 4b64c2a Compare December 9, 2022 16:13
@bikallem
Copy link
Contributor Author

bikallem commented Dec 9, 2022

It's not actually necessary to create a separate Mock.Net.stream_socket type (which is an API change). We could just have Flow.t implement the socket methods too. It already works as both a source and a sink, for example.

We could just add the new method to the implementations in eio_linux, etc, rather than using inheritance, to shrink the PR. Though this seems fine to me either way.

Indeed, this is an api change; however, I think this api addition should be mostly backwards compatible since I am not sure if there is too much (any?) code in the wild which inherits from Eio.Net.socket class.

Additionally and indeed, I agonized a bit on where to put the new function - Eio.Flow.two_wya, Eio.Flow.source or Eio.Flow.sink. However, this line made the decision for me (https://github.com/ocaml-multicore/eio/blob/main/lib_eio/flow.mli#L1). IIUC, flow types are a general/common abstraction over files and network sockets. As such it didn't seem coherent/cogent to me to put it in one of the flow types, i.e. how would we implement setsockopt on file flow type? Eio.Net.socket seemed to fit the requirement as it is a super class for both udp and tcp socket and is an abstraction for sockets.

@bikallem
Copy link
Contributor Author

bikallem commented Dec 9, 2022

Ah, just realized you meant only the mock flow type. I don't really have a strong preference for either in the mock case. I think I mostly followed the abstraction is the Eio.Net.socket as I didn't want to muddy the semantics even if it is for mock use case.

@bikallem
Copy link
Contributor Author

@talex5 @haesbaert please approve/merge this PR is there are no more objections.

@bikallem
Copy link
Contributor Author

closing this for now.

@bikallem bikallem closed this Mar 24, 2023
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.

Support for socket options
3 participants