Repository navigation
Conversation
bf65c3e to
1696dca
Compare
|
@florimondmanca Here is the new PR for #283 where connections are being opened in the connection pool. Although the test doesn't work even now. After setting up the |
|
Hi @gdhameeja, thanks for your PR. Your branch includes a very large I suggest creating a new branch from master and applying those changes to ensure a clean PR. |
|
An alternative to revert without creating a new PR is to edit the commit: $ rm -rf TAGS
$ git add -A
$ git commit --amend --no-edit
$ git push --forceSeems like |
|
No, editor specific files should be in your own personal gitignore file. |
lovelydinosaur
left a comment
lovelydinosaur
left a comment
There was a problem hiding this comment.
Good start! Coupla comments here. (Plus agreed with others re. removing the TAGS being neccessary)
lovelydinosaur
changed the title
|
"An alternative to revert without creating a new PR is to edit the commit" We squash and merge all PRs, so I don't think there's any need to remove it from the history. Just git delete it and we're good with keeping the rest of the pull request as it is. |
1696dca to
d12855c
Compare
d12855c to
a1afc1d
Compare
|
Awaiting review. |
|
@gdhameeja The test is failing because This is looking okay to me in principle, but the mock test isn't convincing me that this will effectively allow HTTPX to handle the case in the wild. Have you thought of a way we could (manually?) test this against a live server? |
florimondmanca
left a comment
florimondmanca
left a comment
There was a problem hiding this comment.
Just a nit, otherwise the code looks good to me. :)
Co-Authored-By: Florimond Manca <florimond.manca@gmail.com>
|
@florimondmanca Currently I am unable to think of a way to test this manually. I might be wrong here but someone mentioned that we can have 2^31 labelled stream ids. With that condition, I think it'd be hard to test this manually. |
|
@florimondmanca @sethmlarson @tomchristie Awaiting review. |
florimondmanca
left a comment
florimondmanca
left a comment
There was a problem hiding this comment.
There are a few issues I hadn't seen before, I left a few hints on how to resolve them. We need to get the build to pass before considering merging this. There have been some changes to the build setup (e.g. Python 3.8 is now mandatory) so you could rebase your changes from master to get the most up to date version?
| except NewConnectionRequired: | ||
| return await self.send( | ||
| request=request, verify=verify, cert=cert, timeout=timeout | ||
| ) |
There was a problem hiding this comment.
It seems this block will actually never run, because it is shadowed by the catch-all block for BaseException — we need to move it above the one for BaseException.
| server.conn.HIGHEST_ALLOWED_STREAM_ID = 1 | ||
|
|
||
| async with httpx.ConnectionPool(backend=backend) as http: | ||
| response = await http.request("GET", server.url) |
There was a problem hiding this comment.
The mock server has no .url attribute, which makes the build fail. I think you can use anything here, since we don't make real requests, so e.g. "/".
|
Sorry to see that this PR has be dragging a bit, I'll close it for housekeeping purposes but we can obviously revisit later with a clean slate. Thanks! |
Bumps [pytest-asyncio](https://github.com/pytest-dev/pytest-asyncio) from 0.15.1 to 0.16.0. - [Release notes](https://github.com/pytest-dev/pytest-asyncio/releases) - [Commits](pytest-dev/pytest-asyncio@v0.15.1...v0.16.0) --- updated-dependencies: - dependency-name: pytest-asyncio dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Tom Christie <tom@tomchristie.com>
Fixes #283
Create new HTTPConnection if stream ids expire. The making of new connection is done in ConnectionPool instead of in HTTPConnection.