Connection: keep-alive usually will only hold the connection for N requests (order 100), so we need some kind of logic to reconnect if write_request() fails once.

I had trouble getting a test to return an appropriate error from write_request when closing the stream. I was able to get an unexpected EOF from read_response though. I made the error check around both instead, reconnecting and retrying when detected.

BTW, the REST and RPC interfaces were creating new connections for each request. I updated them to maintain the connection instead.

tokio doesn't seem like the most stable, any reason to not just pin to a specific version?

I kept it matching lightning-net-tokio but did run into some trouble with dependency version inconsistency when needing serde later on.

@TheBlueMatt Was 0.2.12 chosen for a reason? Presumably there is some bug fix in there. Should we pin on that? I think ">=0.2.12" won't bump us up to 0.3.0 at very least, IIUC.

This matches what we use in lightning-net-tokio which is important. That said, we should just upgrade both to 1.0 and move on.

Updated tokio to 1.0. However, I was unable to get it working with lightning-net-tokio. The test would hang during connection setup.

Initial investigation seemed to show it stuck waiting on tokio::select!. I tried both 1.0 and 0.3 (the public beta for 1.0) and had the same results.

Any insight?

It'd be cool to have CI to spin up a bitcoind node and test these. Maybe too much for this PR though.

Could maybe add a success test case for the non-tokio version.

IIUC, this is using the tokio runtime to drive the test. Otherwise, it will fail when running the test with the tokio feature.

---- http::client_tests::connect_with_valid_endpoint stdout ----
thread 'http::client_tests::connect_with_valid_endpoint' panicked at 'there is no reactor running, must be called from the context of Tokio runtime', /Users/jkczyz/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.0.2/src/io/driver/mod.rs:262:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

However, the non-tokio version is tested still as long as the tokio feature is not specified when running the test.

Should the tests be run at some point? I could be missing something but I can't find them in CI output... Also http::client_tests::connect_to_unresolvable_host's failing for me locally.

Should the tests be run at some point? I could be missing something but I can't find them in CI output...

Whoops! That also may be why there's coverage failures. :P

Also http::client_tests::connect_to_unresolvable_host's failing for me locally.

Are you seeing "Expected error" or something else?

Are you seeing "Expected error" or something else?

Hmm it seems to be passing now, weird. I'll let you know if I can reproduce it again but it's probably my mistake.

If we wanted to save some LoC, doesn't look like this is used and I don't think Core has any API that takes post requests? Edit: there's also a warning in CI output because it's unused.

If we wanted to save some LoC, doesn't look like this is used and I don't think Core has any API that takes post requests?

This is used by RpcClient::call_method as the client uses the HTTP request body to specify the RPC using JSON.

Edit: there's also a warning in CI output because it's unused.

I did see some warnings in intermediary commits but not from HEAD. Looks like this is because the methods aren't used until the implementations of BlockSource are added in a later commit.

This is used by RpcClient::call_method as the client uses the HTTP request body to specify the RPC using JSON.

Missed this!
As discussed, there still seem to be some warnings.

Not 100% sure but I think supporting HTTPS/TLS may need extra work and could be put off.
For example:

	#[tokio::test]
	async fn test_https_servers() {
		let endpoint = HttpEndpoint::secure_host("www.facebook.com".to_string());
		let mut client = HttpClient::connect(&endpoint).unwrap();
		let res = client.get::<BinaryResponse>("/", "facebook.com").await.unwrap();
	}

Seems to result in read_line! reading something binary that isn't what curl returns, so I think the TLS isn't being handled? Also other HTTP client implementations seem to be doing extra work for TLS (e.g. https://docs.rs/native-tls/0.2.7/native_tls/#examples).

If I'm off here, I still think it could be worth adding a test for both HTTP and HTTPS clients using google.com (as opposed to just the test HttpServer).

Not 100% sure but I think supporting HTTPS/TLS may need extra work and could be put off.

Ah, no, you're right. Thanks for testing this case! I remember running across this awhile ago but it fell off my radar. I'll just remove support for it and add a TODO. It would require refactoring the HttpClient API a bit.

If I'm off here, I still think it could be worth adding a test for both HTTP and HTTPS clients using google.com (as opposed to just the test HttpServer).

Generally, I'd prefer to make unit tests hermetic whenever possible to avoid flakiness with the external world.

Having separate tests would be useful, though. I would add a placeholder now to demonstrate that it's currently not supported, but that isn't possible with the current API taking E: ToSocketAddrs.

Generally, I'd prefer to make unit tests hermetic whenever possible to avoid flakiness with the external world.

I've come to agree with this, like we wouldn't want to require an internet connection to run our tests.

Having separate tests would be useful, though. I would add a placeholder now to demonstrate that it's currently not supported, but that isn't possible with the current API taking E: ToSocketAddrs.

Hmm should've asked in the call but I wasn't sure what this meant. Separate tests for what? and what does "placeholder" mean?

Are you seeing "Expected error" or something else?

Hmm it seems to be passing now, weird. I'll let you know if I can reproduce it again but it's probably my mistake.

I'm a bit in favor of just ripping all of the TLS-related out and adding all of it later when we can actually support it (e.g. the presence of Scheme could be confusing for future readers who then assume we support HTTPS).

As discussed offline, could add a test for the try_into() here.

This is used by RpcClient::call_method as the client uses the HTTP request body to specify the RPC using JSON.

Missed this!
As discussed, there still seem to be some warnings.

In my testing, frequently I wanted to know what the actual error was (like EOF) and added print statements to retrieve it. So, might be slightly nicer if Transient contained the actual error. Not a big deal though.

Not sure if conclusions were reached, but we discussed how the ID wasn't being fully utilized in the current PRs and could potentially be pushed off.

I forget if there was any conclusion but the .0 was a bit mysterious / maybe a comment about the tuple it comes from could be nice.

Generally, I'd prefer to make unit tests hermetic whenever possible to avoid flakiness with the external world.

I've come to agree with this, like we wouldn't want to require an internet connection to run our tests.

Having separate tests would be useful, though. I would add a placeholder now to demonstrate that it's currently not supported, but that isn't possible with the current API taking E: ToSocketAddrs.

Hmm should've asked in the call but I wasn't sure what this meant. Separate tests for what? and what does "placeholder" mean?