Add API for poisoning connections #121
Conversation
|
It at least looks like the failed test is relevant. Dropping a client should close its idle connections. |
|
Yeah I agree. I tried pretty hard to reproduce it on a number of different environments but I haven't been able yet. I'll transition this to Draft in the interim |
|
I tried running these tests in Docker and in my dev desktop but couldn't reproduce. :( Running clippy on the tests shows that we're holding a I also see that |
|
I just retried CI, and it's red this time too. |
tests/legacy_client.rs
Outdated
| if sock.read(&mut buf).expect("read 1") > 0 { | ||
| sock.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n") | ||
| .expect("write 1"); | ||
| num_requests_tracker.fetch_add(1, Ordering::Relaxed); |
There was a problem hiding this comment.
Looks like moving num_requests_tracker.fetch_add up before socket.write_all fixes the test.
|
@rcoh mind pushing the change i mentioned ^ so this can be merged |
Co-authored-by: Sean McArthur <[email protected]>
|
tests pass on my machine. not sure why this tests are flaky, can u try rerunning them? |
|
I did re-run it once more after the latest commit. |
|
When this first happened I tried really hard to reproduce. I even pulled down the docker image that they are running in, ran them on multiple host platforms etc. I ran them thousands of times. I was totally unable to reproduce. I'm not sure what's going on here. |
|
Did you run it with |
|
Yes |
|
I took a closer look at this this morning, and realized that the failing test was unrelated to the rest of the code in this PR. After yet another restart, it worked just fine. Looking at the implementation of the tests, they're, uh, wow. Clearly written before async was a thing, and just barely adjust to work with async hyper. It'd be nice to update them to use Anywho, since this works, I'm merging and it'll be in the next release (aiming for today). |
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> #1925 ## Description Backports connection poisoning that hyper 0.14 HTTP client has to the hyper 1.x client. See also: * upstream support: hyperium/hyper-util#121 ## Testing <!--- Please describe in detail how you tested your changes --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Checklist <!--- If a checkbox below is not applicable, then please DELETE it rather than leaving it unchecked --> - [X] For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the `.changelog` directory, specifying "client," "server," or both in the `applies_to` key. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
This is a port of https://github.com/hyperium/hyper/pull/3145/files + a test.
It introduces a
PoisonPillatomic onto connection info. When set to true, this prevents the connection from being returned to the pool.