Update tokio to 1.0 by TheBlueMatt · Pull Request #781 · lightningdevkit/rust-lightning

TheBlueMatt · 2021-01-26T20:40:55Z

This requires ensuring TcpStreams are set in nonblocking mode as
tokio doesn't handle this for us anymore, so we adapt the public
API to just accept std TcpStreams instead of an extra conversion
hop. Luckily converting them is cheap.

This requires ensuring TcpStreams are set in nonblocking mode as tokio doesn't handle this for us anymore, so we adapt the public API to just accept std TcpStreams instead of an extra conversion hop. Luckily converting them is cheap.

jkczyz · 2021-01-27T00:11:01Z

+ with:
+ fetch-depth: 0


The error message may have been a red herring. Instead, the location of the coverage data may be different in Rust 1.45 as @valentinewallace pointed out: #774 (comment)

jkczyz · 2021-01-27T00:54:55Z

+ stream.set_nonblocking(true).unwrap();
+ let (reader, writer) = io::split(TcpStream::from_std(stream).unwrap());


IIUC, the problem was that our test was using std::net::TcpStream::connect without setting the result to non-blocking before converting it to tokio::net::TcpStream.
Could we fix the problem by updating the test to use tokio::net::TcpStream::connect instead? Then a conversion wouldn't be needed here nor in connect_outbound.

I'd initially gone with tokio::net::TcpStream in the public API but then realized its much neater to do the set_nonblocking call in Connection::new given its pretty easy to accidentally do the same issue again, and its simpler for our users to not have to think about it than not. Given that, its fewer conversion calls to just accept an std::net::TcpStream instead of a tokio one. It doesn't really matter, though, just pretty poor API design on Tokio's part.

Rustc 1.45 moved the paths to test binaries, so we need to update our CI scripts to run the correct ones under kcov. The solution to this was pointed out by Val at lightningdevkit#774 (comment)

codecov · 2021-01-27T03:04:35Z

Codecov Report

Merging #781 (0809662) into main (f151c02) will decrease coverage by 0.49%.
The diff coverage is 93.75%.

@@ Coverage Diff @@## main #781 +/- ##
==========================================
- Coverage 91.29% 90.79% -0.50% 
==========================================
Files 37 38 +1 Lines 22921 23168 +247 ==========================================
+ Hits 20925 21036 +111 - Misses 1996 2132 +136

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`49.72% <ø> (-1.30%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.36% <93.75%> (ø)`
lightning/src/util/logger.rs	`57.14% <0.00%> (-2.54%)`	⬇️
lightning/src/ln/wire.rs	`59.24% <0.00%> (-2.37%)`	⬇️
lightning/src/util/config.rs	`48.78% <0.00%> (-1.22%)`	⬇️
lightning/src/routing/network_graph.rs	`90.97% <0.00%> (-1.22%)`	⬇️
lightning/src/util/macro_logger.rs	`88.33% <0.00%> (-0.96%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.85% <0.00%> (-0.69%)`	⬇️
lightning/src/chain/chainmonitor.rs	`94.11% <0.00%> (-0.55%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f151c02...32abba7. Read the comment docs.

TheBlueMatt · 2021-02-02T21:44:09Z

Not sure why CI had refused to run previously, but this is pretty trivial and the last commit is completely trivial. Going to merge.

TheBlueMatt · 2021-01-26T20:40:55Z

This requires ensuring TcpStreams are set in nonblocking mode as
tokio doesn't handle this for us anymore, so we adapt the public
API to just accept std TcpStreams instead of an extra conversion
hop. Luckily converting them is cheap.

This requires ensuring TcpStreams are set in nonblocking mode as tokio doesn't handle this for us anymore, so we adapt the public API to just accept std TcpStreams instead of an extra conversion hop. Luckily converting them is cheap.

jkczyz · 2021-01-27T00:11:01Z

+ with:
+ fetch-depth: 0


The error message may have been a red herring. Instead, the location of the coverage data may be different in Rust 1.45 as @valentinewallace pointed out: #774 (comment)

jkczyz · 2021-01-27T00:54:55Z

+ stream.set_nonblocking(true).unwrap();
+ let (reader, writer) = io::split(TcpStream::from_std(stream).unwrap());


IIUC, the problem was that our test was using std::net::TcpStream::connect without setting the result to non-blocking before converting it to tokio::net::TcpStream.
Could we fix the problem by updating the test to use tokio::net::TcpStream::connect instead? Then a conversion wouldn't be needed here nor in connect_outbound.

I'd initially gone with tokio::net::TcpStream in the public API but then realized its much neater to do the set_nonblocking call in Connection::new given its pretty easy to accidentally do the same issue again, and its simpler for our users to not have to think about it than not. Given that, its fewer conversion calls to just accept an std::net::TcpStream instead of a tokio one. It doesn't really matter, though, just pretty poor API design on Tokio's part.

Rustc 1.45 moved the paths to test binaries, so we need to update our CI scripts to run the correct ones under kcov. The solution to this was pointed out by Val at lightningdevkit#774 (comment)

codecov · 2021-01-27T03:04:35Z

Codecov Report

Merging #781 (0809662) into main (f151c02) will decrease coverage by 0.49%.
The diff coverage is 93.75%.

@@ Coverage Diff @@## main #781 +/- ##
==========================================
- Coverage 91.29% 90.79% -0.50% 
==========================================
Files 37 38 +1 Lines 22921 23168 +247 ==========================================
+ Hits 20925 21036 +111 - Misses 1996 2132 +136

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`49.72% <ø> (-1.30%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.36% <93.75%> (ø)`
lightning/src/util/logger.rs	`57.14% <0.00%> (-2.54%)`	⬇️
lightning/src/ln/wire.rs	`59.24% <0.00%> (-2.37%)`	⬇️
lightning/src/util/config.rs	`48.78% <0.00%> (-1.22%)`	⬇️
lightning/src/routing/network_graph.rs	`90.97% <0.00%> (-1.22%)`	⬇️
lightning/src/util/macro_logger.rs	`88.33% <0.00%> (-0.96%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.85% <0.00%> (-0.69%)`	⬇️
lightning/src/chain/chainmonitor.rs	`94.11% <0.00%> (-0.55%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f151c02...32abba7. Read the comment docs.

TheBlueMatt · 2021-02-02T21:44:09Z

Not sure why CI had refused to run previously, but this is pretty trivial and the last commit is completely trivial. Going to merge.

TheBlueMatt · 2021-01-26T20:40:55Z

This requires ensuring TcpStreams are set in nonblocking mode as
tokio doesn't handle this for us anymore, so we adapt the public
API to just accept std TcpStreams instead of an extra conversion
hop. Luckily converting them is cheap.

This requires ensuring TcpStreams are set in nonblocking mode as tokio doesn't handle this for us anymore, so we adapt the public API to just accept std TcpStreams instead of an extra conversion hop. Luckily converting them is cheap.

jkczyz · 2021-01-27T00:11:01Z

+ with:
+ fetch-depth: 0


The error message may have been a red herring. Instead, the location of the coverage data may be different in Rust 1.45 as @valentinewallace pointed out: #774 (comment)

jkczyz · 2021-01-27T00:54:55Z

+ stream.set_nonblocking(true).unwrap();
+ let (reader, writer) = io::split(TcpStream::from_std(stream).unwrap());


IIUC, the problem was that our test was using std::net::TcpStream::connect without setting the result to non-blocking before converting it to tokio::net::TcpStream.
Could we fix the problem by updating the test to use tokio::net::TcpStream::connect instead? Then a conversion wouldn't be needed here nor in connect_outbound.

I'd initially gone with tokio::net::TcpStream in the public API but then realized its much neater to do the set_nonblocking call in Connection::new given its pretty easy to accidentally do the same issue again, and its simpler for our users to not have to think about it than not. Given that, its fewer conversion calls to just accept an std::net::TcpStream instead of a tokio one. It doesn't really matter, though, just pretty poor API design on Tokio's part.

Rustc 1.45 moved the paths to test binaries, so we need to update our CI scripts to run the correct ones under kcov. The solution to this was pointed out by Val at lightningdevkit#774 (comment)

codecov · 2021-01-27T03:04:35Z

Codecov Report

Merging #781 (0809662) into main (f151c02) will decrease coverage by 0.49%.
The diff coverage is 93.75%.

@@ Coverage Diff @@## main #781 +/- ##
==========================================
- Coverage 91.29% 90.79% -0.50% 
==========================================
Files 37 38 +1 Lines 22921 23168 +247 ==========================================
+ Hits 20925 21036 +111 - Misses 1996 2132 +136

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`49.72% <ø> (-1.30%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.36% <93.75%> (ø)`
lightning/src/util/logger.rs	`57.14% <0.00%> (-2.54%)`	⬇️
lightning/src/ln/wire.rs	`59.24% <0.00%> (-2.37%)`	⬇️
lightning/src/util/config.rs	`48.78% <0.00%> (-1.22%)`	⬇️
lightning/src/routing/network_graph.rs	`90.97% <0.00%> (-1.22%)`	⬇️
lightning/src/util/macro_logger.rs	`88.33% <0.00%> (-0.96%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.85% <0.00%> (-0.69%)`	⬇️
lightning/src/chain/chainmonitor.rs	`94.11% <0.00%> (-0.55%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f151c02...32abba7. Read the comment docs.

TheBlueMatt · 2021-02-02T21:44:09Z

Not sure why CI had refused to run previously, but this is pretty trivial and the last commit is completely trivial. Going to merge.

TheBlueMatt · 2021-01-26T20:40:55Z

This requires ensuring TcpStreams are set in nonblocking mode as
tokio doesn't handle this for us anymore, so we adapt the public
API to just accept std TcpStreams instead of an extra conversion
hop. Luckily converting them is cheap.

This requires ensuring TcpStreams are set in nonblocking mode as tokio doesn't handle this for us anymore, so we adapt the public API to just accept std TcpStreams instead of an extra conversion hop. Luckily converting them is cheap.

jkczyz · 2021-01-27T00:11:01Z

+ with:
+ fetch-depth: 0


The error message may have been a red herring. Instead, the location of the coverage data may be different in Rust 1.45 as @valentinewallace pointed out: #774 (comment)

jkczyz · 2021-01-27T00:54:55Z

+ stream.set_nonblocking(true).unwrap();
+ let (reader, writer) = io::split(TcpStream::from_std(stream).unwrap());


IIUC, the problem was that our test was using std::net::TcpStream::connect without setting the result to non-blocking before converting it to tokio::net::TcpStream.
Could we fix the problem by updating the test to use tokio::net::TcpStream::connect instead? Then a conversion wouldn't be needed here nor in connect_outbound.

I'd initially gone with tokio::net::TcpStream in the public API but then realized its much neater to do the set_nonblocking call in Connection::new given its pretty easy to accidentally do the same issue again, and its simpler for our users to not have to think about it than not. Given that, its fewer conversion calls to just accept an std::net::TcpStream instead of a tokio one. It doesn't really matter, though, just pretty poor API design on Tokio's part.

Rustc 1.45 moved the paths to test binaries, so we need to update our CI scripts to run the correct ones under kcov. The solution to this was pointed out by Val at lightningdevkit#774 (comment)

codecov · 2021-01-27T03:04:35Z

Codecov Report

Merging #781 (0809662) into main (f151c02) will decrease coverage by 0.49%.
The diff coverage is 93.75%.

@@ Coverage Diff @@## main #781 +/- ##
==========================================
- Coverage 91.29% 90.79% -0.50% 
==========================================
Files 37 38 +1 Lines 22921 23168 +247 ==========================================
+ Hits 20925 21036 +111 - Misses 1996 2132 +136

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`49.72% <ø> (-1.30%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.36% <93.75%> (ø)`
lightning/src/util/logger.rs	`57.14% <0.00%> (-2.54%)`	⬇️
lightning/src/ln/wire.rs	`59.24% <0.00%> (-2.37%)`	⬇️
lightning/src/util/config.rs	`48.78% <0.00%> (-1.22%)`	⬇️
lightning/src/routing/network_graph.rs	`90.97% <0.00%> (-1.22%)`	⬇️
lightning/src/util/macro_logger.rs	`88.33% <0.00%> (-0.96%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.85% <0.00%> (-0.69%)`	⬇️
lightning/src/chain/chainmonitor.rs	`94.11% <0.00%> (-0.55%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f151c02...32abba7. Read the comment docs.

TheBlueMatt · 2021-02-02T21:44:09Z

Not sure why CI had refused to run previously, but this is pretty trivial and the last commit is completely trivial. Going to merge.

TheBlueMatt · 2021-01-26T20:40:55Z

This requires ensuring TcpStreams are set in nonblocking mode as
tokio doesn't handle this for us anymore, so we adapt the public
API to just accept std TcpStreams instead of an extra conversion
hop. Luckily converting them is cheap.

This requires ensuring TcpStreams are set in nonblocking mode as tokio doesn't handle this for us anymore, so we adapt the public API to just accept std TcpStreams instead of an extra conversion hop. Luckily converting them is cheap.

jkczyz · 2021-01-27T00:11:01Z

+ with:
+ fetch-depth: 0


The error message may have been a red herring. Instead, the location of the coverage data may be different in Rust 1.45 as @valentinewallace pointed out: #774 (comment)

jkczyz · 2021-01-27T00:54:55Z

+ stream.set_nonblocking(true).unwrap();
+ let (reader, writer) = io::split(TcpStream::from_std(stream).unwrap());


IIUC, the problem was that our test was using std::net::TcpStream::connect without setting the result to non-blocking before converting it to tokio::net::TcpStream.
Could we fix the problem by updating the test to use tokio::net::TcpStream::connect instead? Then a conversion wouldn't be needed here nor in connect_outbound.

I'd initially gone with tokio::net::TcpStream in the public API but then realized its much neater to do the set_nonblocking call in Connection::new given its pretty easy to accidentally do the same issue again, and its simpler for our users to not have to think about it than not. Given that, its fewer conversion calls to just accept an std::net::TcpStream instead of a tokio one. It doesn't really matter, though, just pretty poor API design on Tokio's part.

Rustc 1.45 moved the paths to test binaries, so we need to update our CI scripts to run the correct ones under kcov. The solution to this was pointed out by Val at lightningdevkit#774 (comment)

codecov · 2021-01-27T03:04:35Z

Codecov Report

Merging #781 (0809662) into main (f151c02) will decrease coverage by 0.49%.
The diff coverage is 93.75%.

@@ Coverage Diff @@## main #781 +/- ##
==========================================
- Coverage 91.29% 90.79% -0.50% 
==========================================
Files 37 38 +1 Lines 22921 23168 +247 ==========================================
+ Hits 20925 21036 +111 - Misses 1996 2132 +136

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`49.72% <ø> (-1.30%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.36% <93.75%> (ø)`
lightning/src/util/logger.rs	`57.14% <0.00%> (-2.54%)`	⬇️
lightning/src/ln/wire.rs	`59.24% <0.00%> (-2.37%)`	⬇️
lightning/src/util/config.rs	`48.78% <0.00%> (-1.22%)`	⬇️
lightning/src/routing/network_graph.rs	`90.97% <0.00%> (-1.22%)`	⬇️
lightning/src/util/macro_logger.rs	`88.33% <0.00%> (-0.96%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.85% <0.00%> (-0.69%)`	⬇️
lightning/src/chain/chainmonitor.rs	`94.11% <0.00%> (-0.55%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f151c02...32abba7. Read the comment docs.

TheBlueMatt · 2021-02-02T21:44:09Z

Not sure why CI had refused to run previously, but this is pretty trivial and the last commit is completely trivial. Going to merge.

TheBlueMatt · 2021-01-26T20:40:55Z

This requires ensuring TcpStreams are set in nonblocking mode as
tokio doesn't handle this for us anymore, so we adapt the public
API to just accept std TcpStreams instead of an extra conversion
hop. Luckily converting them is cheap.

This requires ensuring TcpStreams are set in nonblocking mode as tokio doesn't handle this for us anymore, so we adapt the public API to just accept std TcpStreams instead of an extra conversion hop. Luckily converting them is cheap.

jkczyz · 2021-01-27T00:11:01Z

+ with:
+ fetch-depth: 0


The error message may have been a red herring. Instead, the location of the coverage data may be different in Rust 1.45 as @valentinewallace pointed out: #774 (comment)

jkczyz · 2021-01-27T00:54:55Z

+ stream.set_nonblocking(true).unwrap();
+ let (reader, writer) = io::split(TcpStream::from_std(stream).unwrap());


IIUC, the problem was that our test was using std::net::TcpStream::connect without setting the result to non-blocking before converting it to tokio::net::TcpStream.
Could we fix the problem by updating the test to use tokio::net::TcpStream::connect instead? Then a conversion wouldn't be needed here nor in connect_outbound.

I'd initially gone with tokio::net::TcpStream in the public API but then realized its much neater to do the set_nonblocking call in Connection::new given its pretty easy to accidentally do the same issue again, and its simpler for our users to not have to think about it than not. Given that, its fewer conversion calls to just accept an std::net::TcpStream instead of a tokio one. It doesn't really matter, though, just pretty poor API design on Tokio's part.

Rustc 1.45 moved the paths to test binaries, so we need to update our CI scripts to run the correct ones under kcov. The solution to this was pointed out by Val at lightningdevkit#774 (comment)

codecov · 2021-01-27T03:04:35Z

Codecov Report

Merging #781 (0809662) into main (f151c02) will decrease coverage by 0.49%.
The diff coverage is 93.75%.

@@ Coverage Diff @@## main #781 +/- ##
==========================================
- Coverage 91.29% 90.79% -0.50% 
==========================================
Files 37 38 +1 Lines 22921 23168 +247 ==========================================
+ Hits 20925 21036 +111 - Misses 1996 2132 +136

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`49.72% <ø> (-1.30%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.36% <93.75%> (ø)`
lightning/src/util/logger.rs	`57.14% <0.00%> (-2.54%)`	⬇️
lightning/src/ln/wire.rs	`59.24% <0.00%> (-2.37%)`	⬇️
lightning/src/util/config.rs	`48.78% <0.00%> (-1.22%)`	⬇️
lightning/src/routing/network_graph.rs	`90.97% <0.00%> (-1.22%)`	⬇️
lightning/src/util/macro_logger.rs	`88.33% <0.00%> (-0.96%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.85% <0.00%> (-0.69%)`	⬇️
lightning/src/chain/chainmonitor.rs	`94.11% <0.00%> (-0.55%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f151c02...32abba7. Read the comment docs.

TheBlueMatt · 2021-02-02T21:44:09Z

Not sure why CI had refused to run previously, but this is pretty trivial and the last commit is completely trivial. Going to merge.

TheBlueMatt · 2021-01-26T20:40:55Z

This requires ensuring TcpStreams are set in nonblocking mode as
tokio doesn't handle this for us anymore, so we adapt the public
API to just accept std TcpStreams instead of an extra conversion
hop. Luckily converting them is cheap.

This requires ensuring TcpStreams are set in nonblocking mode as tokio doesn't handle this for us anymore, so we adapt the public API to just accept std TcpStreams instead of an extra conversion hop. Luckily converting them is cheap.

jkczyz · 2021-01-27T00:11:01Z

+ with:
+ fetch-depth: 0


The error message may have been a red herring. Instead, the location of the coverage data may be different in Rust 1.45 as @valentinewallace pointed out: #774 (comment)

jkczyz · 2021-01-27T00:54:55Z

+ stream.set_nonblocking(true).unwrap();
+ let (reader, writer) = io::split(TcpStream::from_std(stream).unwrap());


IIUC, the problem was that our test was using std::net::TcpStream::connect without setting the result to non-blocking before converting it to tokio::net::TcpStream.
Could we fix the problem by updating the test to use tokio::net::TcpStream::connect instead? Then a conversion wouldn't be needed here nor in connect_outbound.

I'd initially gone with tokio::net::TcpStream in the public API but then realized its much neater to do the set_nonblocking call in Connection::new given its pretty easy to accidentally do the same issue again, and its simpler for our users to not have to think about it than not. Given that, its fewer conversion calls to just accept an std::net::TcpStream instead of a tokio one. It doesn't really matter, though, just pretty poor API design on Tokio's part.

Rustc 1.45 moved the paths to test binaries, so we need to update our CI scripts to run the correct ones under kcov. The solution to this was pointed out by Val at lightningdevkit#774 (comment)

codecov · 2021-01-27T03:04:35Z

Codecov Report

Merging #781 (0809662) into main (f151c02) will decrease coverage by 0.49%.
The diff coverage is 93.75%.

@@ Coverage Diff @@## main #781 +/- ##
==========================================
- Coverage 91.29% 90.79% -0.50% 
==========================================
Files 37 38 +1 Lines 22921 23168 +247 ==========================================
+ Hits 20925 21036 +111 - Misses 1996 2132 +136

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`49.72% <ø> (-1.30%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.36% <93.75%> (ø)`
lightning/src/util/logger.rs	`57.14% <0.00%> (-2.54%)`	⬇️
lightning/src/ln/wire.rs	`59.24% <0.00%> (-2.37%)`	⬇️
lightning/src/util/config.rs	`48.78% <0.00%> (-1.22%)`	⬇️
lightning/src/routing/network_graph.rs	`90.97% <0.00%> (-1.22%)`	⬇️
lightning/src/util/macro_logger.rs	`88.33% <0.00%> (-0.96%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.85% <0.00%> (-0.69%)`	⬇️
lightning/src/chain/chainmonitor.rs	`94.11% <0.00%> (-0.55%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f151c02...32abba7. Read the comment docs.

TheBlueMatt · 2021-02-02T21:44:09Z

Not sure why CI had refused to run previously, but this is pretty trivial and the last commit is completely trivial. Going to merge.

TheBlueMatt · 2021-01-26T20:40:55Z

This requires ensuring TcpStreams are set in nonblocking mode as
tokio doesn't handle this for us anymore, so we adapt the public
API to just accept std TcpStreams instead of an extra conversion
hop. Luckily converting them is cheap.

This requires ensuring TcpStreams are set in nonblocking mode as tokio doesn't handle this for us anymore, so we adapt the public API to just accept std TcpStreams instead of an extra conversion hop. Luckily converting them is cheap.

jkczyz · 2021-01-27T00:11:01Z

+ with:
+ fetch-depth: 0


The error message may have been a red herring. Instead, the location of the coverage data may be different in Rust 1.45 as @valentinewallace pointed out: #774 (comment)

jkczyz · 2021-01-27T00:54:55Z

+ stream.set_nonblocking(true).unwrap();
+ let (reader, writer) = io::split(TcpStream::from_std(stream).unwrap());


IIUC, the problem was that our test was using std::net::TcpStream::connect without setting the result to non-blocking before converting it to tokio::net::TcpStream.
Could we fix the problem by updating the test to use tokio::net::TcpStream::connect instead? Then a conversion wouldn't be needed here nor in connect_outbound.

I'd initially gone with tokio::net::TcpStream in the public API but then realized its much neater to do the set_nonblocking call in Connection::new given its pretty easy to accidentally do the same issue again, and its simpler for our users to not have to think about it than not. Given that, its fewer conversion calls to just accept an std::net::TcpStream instead of a tokio one. It doesn't really matter, though, just pretty poor API design on Tokio's part.

Rustc 1.45 moved the paths to test binaries, so we need to update our CI scripts to run the correct ones under kcov. The solution to this was pointed out by Val at lightningdevkit#774 (comment)

codecov · 2021-01-27T03:04:35Z

Codecov Report

Merging #781 (0809662) into main (f151c02) will decrease coverage by 0.49%.
The diff coverage is 93.75%.

@@ Coverage Diff @@## main #781 +/- ##
==========================================
- Coverage 91.29% 90.79% -0.50% 
==========================================
Files 37 38 +1 Lines 22921 23168 +247 ==========================================
+ Hits 20925 21036 +111 - Misses 1996 2132 +136

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`49.72% <ø> (-1.30%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.36% <93.75%> (ø)`
lightning/src/util/logger.rs	`57.14% <0.00%> (-2.54%)`	⬇️
lightning/src/ln/wire.rs	`59.24% <0.00%> (-2.37%)`	⬇️
lightning/src/util/config.rs	`48.78% <0.00%> (-1.22%)`	⬇️
lightning/src/routing/network_graph.rs	`90.97% <0.00%> (-1.22%)`	⬇️
lightning/src/util/macro_logger.rs	`88.33% <0.00%> (-0.96%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.85% <0.00%> (-0.69%)`	⬇️
lightning/src/chain/chainmonitor.rs	`94.11% <0.00%> (-0.55%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f151c02...32abba7. Read the comment docs.

TheBlueMatt · 2021-02-02T21:44:09Z

Not sure why CI had refused to run previously, but this is pretty trivial and the last commit is completely trivial. Going to merge.

TheBlueMatt mentioned this pull request Jan 26, 2021
HTTP-based block source clients #774
Merged

jkczyz self-requested a review January 26, 2021 20:52

TheBlueMatt force-pushed the 2021-01-tokio-1 branch from e6e48df to 9fb34feCompare January 26, 2021 23:02

TheBlueMatt added 2 commits January 26, 2021 18:37

Update tokio to 1.0
5a403bd
This requires ensuring TcpStreams are set in nonblocking mode as tokio doesn't handle this for us anymore, so we adapt the public API to just accept std TcpStreams instead of an extra conversion hop. Luckily converting them is cheap.

Bump MSRV for net-tokio to 1.45 as tokio 1.0 req very recent rustc
07aff06

TheBlueMatt force-pushed the 2021-01-tokio-1 branch from 9fb34fe to d6b2f0fCompare January 26, 2021 23:37

jkczyz reviewed Jan 27, 2021
View reviewed changes

TheBlueMatt force-pushed the 2021-01-tokio-1 branch 2 times, most recently from 6affd0b to bf45c98Compare January 27, 2021 02:52

Fix codecov by calling the new binary paths from rust 1.45
11f5e23
Rustc 1.45 moved the paths to test binaries, so we need to update our CI scripts to run the correct ones under kcov. The solution to this was pointed out by Val at lightningdevkit#774 (comment)

TheBlueMatt force-pushed the 2021-01-tokio-1 branch from bf45c98 to 11f5e23Compare January 27, 2021 02:53

jkczyz approved these changes Jan 27, 2021
View reviewed changes

Drop spurious semicolon that new rustc complains about
32abba7

TheBlueMatt force-pushed the 2021-01-tokio-1 branch from 6a90fbb to 32abba7Compare February 2, 2021 21:43

TheBlueMatt merged commit 1ce1902 into lightningdevkit:mainFeb 2, 2021