Drop dep `tokio`'s `io-util` feat as it broke MSRV and isn't useful by TheBlueMatt · Pull Request #2537 · lightningdevkit/rust-lightning

TheBlueMatt · 2023-08-28T18:46:19Z

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 · 2023-08-28T19:46:47Z

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 · 2023-08-28T20:49:47Z

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/github.com-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 · 2023-08-28T21:06:08Z

Grr rust-bitcoin/rust-bitcoin#2034

TheBlueMatt · 2023-08-28T21:07:31Z

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

wpaulino · 2023-08-28T21:14:37Z

Feel free to squash, LGTM

jkczyz

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. Fixeslightningdevkit#2527.

TheBlueMatt · 2023-08-28T21:17:39Z

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 · 2023-08-28T18:46:19Z

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 · 2023-08-28T19:46:47Z

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 · 2023-08-28T20:49:47Z

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/github.com-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 · 2023-08-28T21:06:08Z

Grr rust-bitcoin/rust-bitcoin#2034

TheBlueMatt · 2023-08-28T21:07:31Z

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

wpaulino · 2023-08-28T21:14:37Z

Feel free to squash, LGTM

jkczyz

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. Fixeslightningdevkit#2527.

TheBlueMatt · 2023-08-28T21:17:39Z

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 · 2023-08-28T18:46:19Z

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 · 2023-08-28T19:46:47Z

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 · 2023-08-28T20:49:47Z

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/github.com-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 · 2023-08-28T21:06:08Z

Grr rust-bitcoin/rust-bitcoin#2034

TheBlueMatt · 2023-08-28T21:07:31Z

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

wpaulino · 2023-08-28T21:14:37Z

Feel free to squash, LGTM

jkczyz

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. Fixeslightningdevkit#2527.

TheBlueMatt · 2023-08-28T21:17:39Z

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 · 2023-08-28T18:46:19Z

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 · 2023-08-28T19:46:47Z

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 · 2023-08-28T20:49:47Z

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/github.com-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 · 2023-08-28T21:06:08Z

Grr rust-bitcoin/rust-bitcoin#2034

TheBlueMatt · 2023-08-28T21:07:31Z

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

wpaulino · 2023-08-28T21:14:37Z

Feel free to squash, LGTM

jkczyz

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. Fixeslightningdevkit#2527.

TheBlueMatt · 2023-08-28T21:17:39Z

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 · 2023-08-28T18:46:19Z

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 · 2023-08-28T19:46:47Z

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 · 2023-08-28T20:49:47Z

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/github.com-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 · 2023-08-28T21:06:08Z

Grr rust-bitcoin/rust-bitcoin#2034

TheBlueMatt · 2023-08-28T21:07:31Z

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

wpaulino · 2023-08-28T21:14:37Z

Feel free to squash, LGTM

jkczyz

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. Fixeslightningdevkit#2527.

TheBlueMatt · 2023-08-28T21:17:39Z

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 · 2023-08-28T18:46:19Z

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 · 2023-08-28T19:46:47Z

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 · 2023-08-28T20:49:47Z

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/github.com-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 · 2023-08-28T21:06:08Z

Grr rust-bitcoin/rust-bitcoin#2034

TheBlueMatt · 2023-08-28T21:07:31Z

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

wpaulino · 2023-08-28T21:14:37Z

Feel free to squash, LGTM

jkczyz

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. Fixeslightningdevkit#2527.

TheBlueMatt · 2023-08-28T21:17:39Z

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 · 2023-08-28T18:46:19Z

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 · 2023-08-28T19:46:47Z

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 · 2023-08-28T20:49:47Z

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/github.com-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 · 2023-08-28T21:06:08Z

Grr rust-bitcoin/rust-bitcoin#2034

TheBlueMatt · 2023-08-28T21:07:31Z

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

wpaulino · 2023-08-28T21:14:37Z

Feel free to squash, LGTM

jkczyz

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. Fixeslightningdevkit#2527.

TheBlueMatt · 2023-08-28T21:17:39Z

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 · 2023-08-28T18:46:19Z

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 · 2023-08-28T19:46:47Z

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 · 2023-08-28T20:49:47Z

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/github.com-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 · 2023-08-28T21:06:08Z

Grr rust-bitcoin/rust-bitcoin#2034

TheBlueMatt · 2023-08-28T21:07:31Z

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

wpaulino · 2023-08-28T21:14:37Z

Feel free to squash, LGTM

jkczyz

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. Fixeslightningdevkit#2527.

TheBlueMatt · 2023-08-28T21:17:39Z

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.
$

jkczyz mentioned this pull request Aug 28, 2023
CI failures running Rust 1.48 and 1.49 on ubuntu-latest and windows-latest #2527
Open

wpaulino reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 1046f33 to 97b6c6fCompare August 28, 2023 21:07

jkczyz reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt added 2 commits August 28, 2023 21:16

Pin the memchr test-only transitive dependency of regex
afc5a02

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 97b6c6f to afc5a02Compare August 28, 2023 21:16

wpaulino approved these changes Aug 28, 2023
View reviewed changes

jkczyz approved these changes Aug 28, 2023
View reviewed changes

TheBlueMatt merged commit e57fbba into lightningdevkit:mainAug 29, 2023

jkczyz mentioned this pull request Aug 28, 2023
CI failures running Rust 1.48 and 1.49 on ubuntu-latest and windows-latest #2527
Open

wpaulino reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 1046f33 to 97b6c6fCompare August 28, 2023 21:07

jkczyz reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt added 2 commits August 28, 2023 21:16

Pin the memchr test-only transitive dependency of regex
afc5a02

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 97b6c6f to afc5a02Compare August 28, 2023 21:16

wpaulino approved these changes Aug 28, 2023
View reviewed changes

jkczyz approved these changes Aug 28, 2023
View reviewed changes

TheBlueMatt merged commit e57fbba into lightningdevkit:mainAug 29, 2023

jkczyz mentioned this pull request Aug 28, 2023
CI failures running Rust 1.48 and 1.49 on ubuntu-latest and windows-latest #2527
Open

wpaulino reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 1046f33 to 97b6c6fCompare August 28, 2023 21:07

jkczyz reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt added 2 commits August 28, 2023 21:16

Pin the memchr test-only transitive dependency of regex
afc5a02

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 97b6c6f to afc5a02Compare August 28, 2023 21:16

wpaulino approved these changes Aug 28, 2023
View reviewed changes

jkczyz approved these changes Aug 28, 2023
View reviewed changes

TheBlueMatt merged commit e57fbba into lightningdevkit:mainAug 29, 2023

jkczyz mentioned this pull request Aug 28, 2023
CI failures running Rust 1.48 and 1.49 on ubuntu-latest and windows-latest #2527
Open

wpaulino reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 1046f33 to 97b6c6fCompare August 28, 2023 21:07

jkczyz reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt added 2 commits August 28, 2023 21:16

Pin the memchr test-only transitive dependency of regex
afc5a02

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 97b6c6f to afc5a02Compare August 28, 2023 21:16

wpaulino approved these changes Aug 28, 2023
View reviewed changes

jkczyz approved these changes Aug 28, 2023
View reviewed changes

TheBlueMatt merged commit e57fbba into lightningdevkit:mainAug 29, 2023

jkczyz mentioned this pull request Aug 28, 2023
CI failures running Rust 1.48 and 1.49 on ubuntu-latest and windows-latest #2527
Open

wpaulino reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 1046f33 to 97b6c6fCompare August 28, 2023 21:07

jkczyz reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt added 2 commits August 28, 2023 21:16

Pin the memchr test-only transitive dependency of regex
afc5a02

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 97b6c6f to afc5a02Compare August 28, 2023 21:16

wpaulino approved these changes Aug 28, 2023
View reviewed changes

jkczyz approved these changes Aug 28, 2023
View reviewed changes

TheBlueMatt merged commit e57fbba into lightningdevkit:mainAug 29, 2023

jkczyz mentioned this pull request Aug 28, 2023
CI failures running Rust 1.48 and 1.49 on ubuntu-latest and windows-latest #2527
Open

wpaulino reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 1046f33 to 97b6c6fCompare August 28, 2023 21:07

jkczyz reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt added 2 commits August 28, 2023 21:16

Pin the memchr test-only transitive dependency of regex
afc5a02

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 97b6c6f to afc5a02Compare August 28, 2023 21:16

wpaulino approved these changes Aug 28, 2023
View reviewed changes

jkczyz approved these changes Aug 28, 2023
View reviewed changes

TheBlueMatt merged commit e57fbba into lightningdevkit:mainAug 29, 2023

jkczyz mentioned this pull request Aug 28, 2023
CI failures running Rust 1.48 and 1.49 on ubuntu-latest and windows-latest #2527
Open

wpaulino reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 1046f33 to 97b6c6fCompare August 28, 2023 21:07

jkczyz reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt added 2 commits August 28, 2023 21:16

Pin the memchr test-only transitive dependency of regex
afc5a02

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 97b6c6f to afc5a02Compare August 28, 2023 21:16

wpaulino approved these changes Aug 28, 2023
View reviewed changes

jkczyz approved these changes Aug 28, 2023
View reviewed changes

TheBlueMatt merged commit e57fbba into lightningdevkit:mainAug 29, 2023

jkczyz mentioned this pull request Aug 28, 2023
CI failures running Rust 1.48 and 1.49 on ubuntu-latest and windows-latest #2527
Open

wpaulino reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 1046f33 to 97b6c6fCompare August 28, 2023 21:07

jkczyz reviewed Aug 28, 2023
View reviewed changes

Comment threadlightning-net-tokio/src/lib.rs Outdated

TheBlueMatt added 2 commits August 28, 2023 21:16

Pin the memchr test-only transitive dependency of regex
afc5a02

TheBlueMatt force-pushed the 2023-08-one-less-feature-dep branch from 97b6c6f to afc5a02Compare August 28, 2023 21:16

wpaulino approved these changes Aug 28, 2023
View reviewed changes

jkczyz approved these changes Aug 28, 2023
View reviewed changes

TheBlueMatt merged commit e57fbba into lightningdevkit:mainAug 29, 2023

Uh oh!

Conversation

TheBlueMatt commented Aug 28, 2023

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

jkczyz commented Aug 28, 2023

Uh oh!

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

wpaulino commented Aug 28, 2023

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

TheBlueMatt commented Aug 28, 2023

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

jkczyz commented Aug 28, 2023

Uh oh!

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

wpaulino commented Aug 28, 2023

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

TheBlueMatt commented Aug 28, 2023

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

jkczyz commented Aug 28, 2023

Uh oh!

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

wpaulino commented Aug 28, 2023

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Aug 28, 2023

Uh oh!

Reviewers