Skip to content

Drop dep tokio's io-util feat as it broke MSRV and isn't useful #2537

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

Conversation

TheBlueMatt
Copy link
Collaborator

We use tokio's io-util feature to provide the
Async{Read,Write}Ext traits, which allow us to simply launch a read future or poll_write directly as well as split the TcpStream into a read/write half. However, these traits aren't actually doing much for us - they are really just wrapping the readable future (which we can trivially use ourselves) and poll_write isn't doing anything for us that poll_write_ready can't.

Similarly, the split logic is actually just Arcing the TcpStream and busy-waiting when an operation is busy to prevent concurrent reads/writes. However, there's no reason to prevent concurrent access at the stream level - we aren't ever concurrently writing or reading (though we may concurrently read and write, which is fine).

Worse, the io-util feature broke MSRV (though they're likely to fix this upstream) and carries two additional dependencies (only one on the latest upstream tokio).

Thus, we simply drop the dependency here.

Fixes #2527.

@TheBlueMatt
Copy link
Collaborator Author

Oops, also had to test-only pin memchr anyway cause regex uses it, but that's okay, this PR still removes the dependency on bytes, and memchr on older tokio in non-dev environments.

@jkczyz
Copy link
Contributor

jkczyz commented Aug 28, 2023

CI still unhappy 😭

+ cargo check --verbose --color always
    Updating crates.io index
    Updating git repository `[https://github.com/arik-so/rust-musig2`](https://github.com/arik-so/rust-musig2%60)
 Downloading crates ...
  Downloaded memchr v2.6.0
error: failed to parse manifest at `/home/runner/.cargo/registry/src/i.8713187.xyz-1ecc6299db9ec823/memchr-2.6.0/Cargo.toml`

Caused by:
  failed to parse the `edition` key

Caused by:
  this version of Cargo is older than the `2021` edition, and only supports `2015` and `2018` editions.
Error: Process completed with exit code 101.

@TheBlueMatt
Copy link
Collaborator Author

Grr rust-bitcoin/rust-bitcoin#2034

@TheBlueMatt TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 1046f33 to 97b6c6f Compare August 28, 2023 21:07
@TheBlueMatt
Copy link
Collaborator Author

In any case, IMO we should still land this while we work through core2.

@wpaulino
Copy link
Contributor

Feel free to squash, LGTM

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM

We use `tokio`'s `io-util` feature to provide the
`Async{Read,Write}Ext` traits, which allow us to simply launch a
read future or `poll_write` directly as well as `split` the
`TcpStream` into a read/write half. However, these traits aren't
actually doing much for us - they are really just wrapping the
`readable` future (which we can trivially use ourselves) and
`poll_write` isn't doing anything for us that `poll_write_ready`
can't.

Similarly, the split logic is actually just `Arc`ing the
`TcpStream` and busy-waiting when an operation is busy to prevent
concurrent reads/writes. However, there's no reason to prevent
concurrent access at the stream level - we aren't ever concurrently
writing or reading (though we may concurrently read and write,
which is fine).

Worse, the `io-util` feature broke MSRV (though they're likely to
fix this upstream) and carries two additional dependencies (only
one on the latest upstream tokio).

Thus, we simply drop the dependency here.

Fixes lightningdevkit#2527.
@TheBlueMatt TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 97b6c6f to afc5a02 Compare August 28, 2023 21:16
@TheBlueMatt
Copy link
Collaborator Author

Squashed with the one extra space removed:

$ git diff-tree -U1 97b6c6f6 afc5a02f
diff --git a/lightning-net-tokio/src/lib.rs b/lightning-net-tokio/src/lib.rs
index 55906c64f..6e2ea3f14 100644
--- a/lightning-net-tokio/src/lib.rs
+++ b/lightning-net-tokio/src/lib.rs
@@ -467,3 +467,3 @@ impl peer_handler::SocketDescriptor for SocketDescriptor {
 		// there's room in the kernel buffer, or otherwise create a new Waker with a
-		// SocketDescriptor  in it which can wake up the write_avail Sender, waking up the
+		// SocketDescriptor in it which can wake up the write_avail Sender, waking up the
 		// processing future which will call write_buffer_space_avail and we'll end up back here.
$ 

@TheBlueMatt TheBlueMatt merged commit e57fbba into lightningdevkit:main Aug 29, 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.

CI failures running Rust 1.48 and 1.49 on ubuntu-latest and windows-latest
3 participants