fix(client): race in connection errors propagation

[dare3path]

Fix a race condition in the legacy HTTP client's connection setup where connection errors (e.g., TLS failures, unexpected server responses) were discarded, resulting in vague ChannelClosed errors.

seanmonstar/reqwest#2649

[seanmonstar]

Thanks for fixing this, I now see how this fits together, makes sense!

I wonder, can you think of a way we could deterministically test this? We can control a mock IO pretty strictly with tokio_test::io. It doesn't need to have tests here for me to merge, but it might be a helpful follow up if you're interested, to help prevent relapse as we refactor the legacy client (into a new one, eventually).

[dare3path]

I'll look into it, though I should mention that after something like 2 days or more with grok3 I failed to make tests here in hyper-util(without using reqwest), however since I don't know much, I didn't know about that mock IO that you mentioned, maybe with this new info grok3&i could come up with some reliable tests. I'll try and let you know what comes of it. If in the meantime you decide otherwise, please let me know. I'm on it until then or until something comes of it.

[dare3path]

success

EDIT: I haven't really looked at the code(not that I'd really understand it), it's grok3 generated, but I tested it with and without PR, seems reliable.

• - maybe I should replace some eprintln!() from these tests with trace!() if you're pulling in that 3rd commit too with the tracing subscriber. Otherwise, --nocapture would be required to see those eprintln!(), but not required for trace!() (though RUST_LOG=trace or similar is required for trace!() to show). Though having those be eprintln!() in the tests kinda decouples them visually, so it's obvious that trace! and debug! outputs are from the code and eprintln! are from the tests.

[seanmonstar]

Awesome, glad there's code that can test this! Could you remove the part about using ctor? I don't think we need that. We don't need the init tracing part either. It's fine if those mock types use eprintln, if the test fails stderr will be printed too.

[dare3path]

Could you remove the part about using ctor? I don't think we need that. We don't need the init tracing part either.

done, and rebased on current master (8805922)