Skip to content

fix(client): race in connection errors propagation #184

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dare3path
Copy link

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

@dare3path

This comment was marked as outdated.

@dare3path dare3path force-pushed the fix_ChannelClosed_race branch from f76661f to faeeee6 Compare May 6, 2025 12:30
@seanmonstar
Copy link
Member

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
Copy link
Author

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
Copy link
Author

dare3path commented May 7, 2025

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.

dare3path added 2 commits May 7, 2025 02:40
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
@dare3path dare3path force-pushed the fix_ChannelClosed_race branch from faeeee6 to 9a0b4a3 Compare May 7, 2025 00:41
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.

2 participants