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

Don't require Unix in order to use mock sockets #74

Merged
merged 1 commit into from
Apr 27, 2024

Conversation

quernd
Copy link
Contributor

@quernd quernd commented Jan 23, 2024

Thanks for always being at the forefront and adapting to breaking changes in Eio's API!

After the Eio objects to variants migration, we weren't able to use an Eio_mock.Net network for tests anymore because commit 0c672bf changed the socket type in the Client module from Eio.Flow.two_way to Eio_unix.Net.stream_socket_ty Eio.Net.stream_socket.

This is in contrast to the Server module where it's still _ Eio.Net.stream_socket.

As far as I can tell, there's no necessity for this restriction, so this PR changes it back and exposes socket as Eio.Flow.two_way again. ([ `Generic ] Eio.Net.stream_socket_ty Eio.Net.stream_socket would also be possible and maybe more appropriate; I went with two_way because that's what it was before.)

However I don't know if there is any code that relies on it being Unix instead of the more generic type. I wasn't able to find any, but if this was a conscious choice maybe we can find a different way to allow for mock tests and still expose the socket (I refrained from adding a type parameter to Client.t for the network type but that would be one way, I guess).

If you think this is change feasible, there's a one-line change in h2, httpaf etc. needed as well so that they can also use mock networks again, along the lines of this commit. I can open those PRs but they only make sense in conjunction with this one.

@anmonteiro
Copy link
Owner

I believe my thinking at the time was that two_way sockets didn't provide close, but I could be misremembering. I'll test this soon and let you know.

@quernd
Copy link
Contributor Author

quernd commented Jan 23, 2024

I believe my thinking at the time was that two_way sockets didn't provide close, but I could be misremembering. I'll test this soon and let you know.

Well, you're right: two_way flows don't provide close. If it's important that this be exposed then maybe socket can be coerced to type [ `Generic ] Eio.Net.stream_socket_ty Eio.Net.stream_socket or something similar? It would provide close but not be tied to Unix, and it would also work fine with our mock tests.

@wokalski
Copy link

wokalski commented Apr 16, 2024

[ `Generic ] Eio.Net.stream_socket_ty Eio.Net.stream_socket or [ `Close | Eio.Flow.two_way_ty ] Eio.Flow.two_way or carrying the 'net generic around will all work. depending on the design goals of the val socket function carrying 'net around or coercion is probably preferred.

@wokalski
Copy link

@anmonteiro rebased on top of the latest changes.

@anmonteiro
Copy link
Owner

appreciate your patience here, give me a bit more time.

@anmonteiro anmonteiro merged commit 5097248 into anmonteiro:master Apr 27, 2024
3 checks passed
@anmonteiro
Copy link
Owner

Thanks!

anmonteiro added a commit that referenced this pull request Apr 27, 2024
@wokalski wokalski deleted the allow-mocks branch April 27, 2024 08:56
anmonteiro added a commit to anmonteiro/opam-repository that referenced this pull request Jun 4, 2024
CHANGES:

- gluten-lwt,gluten-eio: remove `Client.socket`
  ([anmonteiro/gluten#75](anmonteiro/gluten#75))
- gluten-eio: don't require `Eio_unix.stream_socket_ty`, allowing the use of
  mock sockets ([anmonteiro/gluten#74](anmonteiro/gluten#74))
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.

None yet

3 participants