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

Create BaseHTTPConnection APIs #2768

Merged
merged 14 commits into from
Nov 10, 2022

Conversation

sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Nov 8, 2022

Closes #1985

Started by taking apart and documenting the HTTP connection (+pool) lifecycle and what would be necessary at every stage. Decided it might be good to document these in notes that we can keep up to date once we settle on an API.

In general I'd like to fix up a bit of our super complicated proxy situation (cc @jalopezsilva) since we technically "own" the HTTPConnection API. Timeouts are similarly complicated but done in multiple ways.

There's some more features I want to dig into and document like HTTPConnection.auto_open which appears to be to do with whether a connection is reusable.

I have some local work creating a BaseHTTPConnection protocol and trying it with type hints but that's not quite ready yet.

pquentin
pquentin previously approved these changes Nov 8, 2022
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, and I love how this highlights improvements that we could carry on today.

The internal property usage is a big problem.

Have you studied HTTP1Connection from the v2 branch and hip? https://github.com/python-trio/hip/blob/e198149c677edbeec023aeb934758c9195a4d2e3/src/ahip/connection.py#L317-L565? I think there are valuable lessons there.

Finally, I think those notes stand on their own, and I would happy to merge them as-is.

notes/connection-lifecycle.md Outdated Show resolved Hide resolved
notes/connection-lifecycle.md Outdated Show resolved Hide resolved
notes/connection-lifecycle.md Outdated Show resolved Hide resolved
@sethmlarson sethmlarson force-pushed the http-connection-api branch 4 times, most recently from 0c64764 to 23f08a3 Compare November 9, 2022 14:28
@sethmlarson sethmlarson marked this pull request as ready for review November 9, 2022 18:09
@sethmlarson
Copy link
Member Author

sethmlarson commented Nov 9, 2022

Noting this down for future me because I have to leave: default port when specifying a scheme in HTTPConnection.set_tunnel().

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work. 👏

changelog/1985.removal.rst Show resolved Hide resolved
.coveragerc Show resolved Hide resolved
ci/0005-botocore-fakesocket-settimeout.patch Show resolved Hide resolved
notes/connection-lifecycle.md Show resolved Hide resolved
src/urllib3/connection.py Show resolved Hide resolved
pquentin
pquentin previously approved these changes Nov 10, 2022
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thank you! I've left a few comments, but this does not need another round of review. 🎉

notes/connection-lifecycle.md Outdated Show resolved Hide resolved
notes/connection-lifecycle.md Outdated Show resolved Hide resolved
notes/connection-lifecycle.md Outdated Show resolved Hide resolved
@vEpiphyte
Copy link

vEpiphyte commented May 3, 2023

For reference this did not document the removal of the VerifiedHTTPSConnection class ( despite its rename in #1805 ) from the connectionpool module.

edit: noted the module detail.

@pquentin
Copy link
Member

pquentin commented May 3, 2023

@vEpiphyte VerifiedHTTPSConnection is still here in urllib3 2.0, we just stopped importing it from connectionpool.py. If this is where you were importing it from, your code was only working by accident.

You can use from urllib3.connection import VerifiedHTTPSConnection instead but of course at this point it would be better to use the new name with from urllib3.connection import HTTPSConnection.

Why did you need this?

@vEpiphyte
Copy link

vEpiphyte commented May 3, 2023

@pquentin Looking into this issue in vcrpy ( which you commented on, thank you very much! ) kevin1024/vcrpy#688 since that has caused an immediate outage for myself and my downstream users.

Almost more so a note for myself, since git bisect & github's compare feature didn't quickly tell me where the removal of the import came from. For example, 2.0.1...1.26.15 does not show the removal of the import from connection.py.

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.

[v2] Create a BaseHTTPConnection APIs
3 participants