Skip to content
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

Ensure pool timeout is applied, even after attempts leading to ConnectionNotAvailable #823

Merged
merged 5 commits into from
Oct 12, 2023

Conversation

valsteen
Copy link
Contributor

@valsteen valsteen commented Oct 11, 2023

Summary

fixes #822

This ensures the total time spent trying to acquire a connection is considered.

  • both sync and async versions are fixed
  • the first commit add failing tests contains a failing test case demonstrating the issue. The second commit fixes it
  • as before, a pool timeout of 0 will succeed if a connection is immediately available

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly – the documentation does not need change

@valsteen valsteen force-pushed the fix-pooltimeout branch 2 times, most recently from ac8bd4c to ee12daa Compare October 11, 2023 06:32
@valsteen valsteen marked this pull request as draft October 11, 2023 06:40
@valsteen valsteen force-pushed the fix-pooltimeout branch 3 times, most recently from 0c49e79 to 10742de Compare October 11, 2023 08:21
@karpetrosyan
Copy link
Member

I believe that changes such as "quote to double quote" are unnecessary in this pull request.
We also want to keep things as simple as possible, so the time module should be removed along with the async_open_nursery function.

The implementation makes sense, but the test cases are overly complicated; we could probably write them much more simply.

@valsteen valsteen force-pushed the fix-pooltimeout branch 2 times, most recently from fc79e17 to 9fa759d Compare October 11, 2023 08:29
@valsteen valsteen force-pushed the fix-pooltimeout branch 3 times, most recently from ce92bcf to cb112af Compare October 11, 2023 09:16
@valsteen valsteen marked this pull request as ready for review October 11, 2023 09:21
@karpetrosyan
Copy link
Member

Let's just use the previous test case.
This test case is complicated enough to be ignored.
Also, I am not sure if these kinds of milisecond timeouts are reliable.

I have tried to write a simple test for these cases, but it seems like we should avoid that.
The implementation makes sense to me; let's just add a changelog, and I think it should be ready.

@valsteen
Copy link
Contributor Author

@karpetrosyan I understand, let me check if it can be isolated/simplified

@valsteen
Copy link
Contributor Author

tests reverted and Changelog added

@karpetrosyan
Copy link
Member

This is great. Thank you @valsteen!

@karpetrosyan karpetrosyan merged commit f8ff1c4 into encode:master Oct 12, 2023
5 checks passed
karpetrosyan pushed a commit to karpetrosyan/httpcore that referenced this pull request Oct 19, 2023
…tionNotAvailable (encode#823)

* add failing tests

* fix pooltimeout

* Revert "add failing tests"

This reverts commit cacc248.

* mark `if timeout < 0` as not covered, since the test is reverted

* Update CHANGELOG.md
This was referenced Nov 2, 2023
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.

Pool timeout doesn't work
2 participants