Skip to content

feat: socket support using LibUV #6683

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

Merged
merged 92 commits into from
Mar 19, 2025

Conversation

algebraic-dev
Copy link
Contributor

@algebraic-dev algebraic-dev commented Jan 17, 2025

This PR introduces TCP socket support using the LibUV library, enabling asynchronous I/O operations with it.

algebraic-dev and others added 11 commits January 10, 2025 16:44
This PR adds support for `Timer` and a event loop thread that gets
requests from another threads and executes it.

---------

Co-authored-by: Markus Himmel <[email protected]>
Co-authored-by: Henrik Böving <[email protected]>
This PR implements a basic asynchronous timer API on top of the libuv
work.

It purposely puts this into `Std.Internal` as we might still have to
change the API as we continue develop of the async library across
releases so I would only like to stabilize it once we are certain this
is a fine API.

A few additional notes:
- we currently do not implement a bind operator on `AsyncTask` on
purpose as `Task.bind` on `Task.pure` is a non trivial operation and
users should be aware of it. Furthermore there is the consideration that
as they will have to bind on both `IO` and `AsyncTask` we might want to
make potential task points explicit in the syntax (did somebody say
`await`?).
- the API generally takes inspiration from
https://docs.rs/tokio/latest/tokio/time/index.html, though it has to
adapt as Rust's and Lean's asynchronity concepts are sufficiently
different.

Stacked on top of leanprover#6219.
@algebraic-dev algebraic-dev requested a review from TwoFX as a code owner January 17, 2025 23:53
@algebraic-dev algebraic-dev marked this pull request as draft January 17, 2025 23:53
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Jan 23, 2025
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Jan 23, 2025

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 9b629cc81f01642200eb0762da39b2935652754c --onto 9b74c07767dc50645efa00356a7724e7f7176227. (2025-01-23 05:57:38)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 104b3519d79f5996dd6d325cba18d94a4615e418 --onto 20c616503abe5ce4253c56dbcd7766a91c675ba0. (2025-01-30 00:33:23)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 104b3519d79f5996dd6d325cba18d94a4615e418 --onto a35bf7ee4c4d900475d88886825a5337f389bd0f. (2025-01-30 00:52:24)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 104b3519d79f5996dd6d325cba18d94a4615e418 --onto e922edfc218090ab2e54092336c67fe3b970dfc2. (2025-01-30 17:26:52)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 104b3519d79f5996dd6d325cba18d94a4615e418 --onto 832d7c500d709deb5e7f0a5a6fd0f01865d1a303. (2025-02-04 04:01:35)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 5eca093a89bc7dcd09e54b0a9c71bb3472d66709 --onto b01ca8ee237a1b3c299384e73ad79d424e216a84. (2025-02-08 00:17:56)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 2fed93462d964252519aa0d2fcd797f86bb52f7c --onto 2960b384af1f29f566f59a3ede8ac982e4628f19. (2025-02-22 00:32:04)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 2fed93462d964252519aa0d2fcd797f86bb52f7c --onto 5cbeb22564c6caa0603612edfdabda4bdb34b780. (2025-02-25 16:51:49)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 2fed93462d964252519aa0d2fcd797f86bb52f7c --onto c3402b85ab96b7ceec32e0b119837913da0051d0. (2025-02-26 12:14:54)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 2fed93462d964252519aa0d2fcd797f86bb52f7c --onto 727c696d9f132bc2701b9755f8819bd36335cd26. (2025-02-27 23:20:20)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 2fed93462d964252519aa0d2fcd797f86bb52f7c --onto ca0d8226192e7c0cdcc71d6322c3226ad4f73f30. You can force Mathlib CI using the force-mathlib-ci label. (2025-03-07 22:02:33)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase a0acbd77ea784d43d8daab052567da0087b3783c --onto d32a7b250ad20477d55034488d89a050fbf70af5. You can force Mathlib CI using the force-mathlib-ci label. (2025-03-18 01:23:29)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 53fcae031e5a37c7d8c0ac880f4da28929e3b932 --onto d32a7b250ad20477d55034488d89a050fbf70af5. You can force Mathlib CI using the force-mathlib-ci label. (2025-03-18 15:55:39)

Copy link
Contributor

@hargoniX hargoniX left a comment

Choose a reason for hiding this comment

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

I think this is very promising now! There's mostly on memory leaks and stylistic things left but the implementation looks like it should be able to work now.

Beyond these issues it would of course be great if we could also have some tests like for Timer that exercise this API. I would assume you can just connect to localhost and do an echo or something like that? Of course we should have tests for both IPv4 and v6.

After we have those tests I guess it might also be a good idea to write a little benchmark to see how many TCP connections we can handle per second to get an estimate of what kind of load we can expect to handle at best given this implementation.

Copy link
Contributor

@hargoniX hargoniX left a comment

Choose a reason for hiding this comment

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

I think the code looks pretty reasonable now, two more things that are generally relevant that I didn't point out:

  • As previously pointed out it would be nice to have some debug_asserts for all of the implicit invariants that are flying around here.
  • As previously pointed out we should make it stylistically consistent, currently the pointer * sometimes goes like type * var and other times like type* var (with the third but unused option of course being type *var). We should stick to one style consistently here, I personally prefer type* but we should make that call based on existing code.
  • I'd like to get a second opinion before we merge the code. I think the implementation is sound logic wise but it could still be possible that we oversaw a failure condition or a memleak.

@TwoFX TwoFX added the release-ci Enable all CI checks for a PR, like is done for releases label Mar 11, 2025
Copy link
Contributor

@hargoniX hargoniX left a comment

Choose a reason for hiding this comment

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

Apart from those few nits it's fine to merge now I think.

@hargoniX hargoniX removed the awaiting-review Waiting for someone to review the PR label Mar 18, 2025
@algebraic-dev algebraic-dev added this pull request to the merge queue Mar 19, 2025
Merged via the queue into leanprover:master with commit d7d1754 Mar 19, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-library Library release-ci Enable all CI checks for a PR, like is done for releases toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants