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

fix: properly close streaming requests when not completely consumed #9899

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

Tasssadar
Copy link
Contributor

https://requests.readthedocs.io/en/latest/user/advanced/#body-content-workflow

If you set stream to True when making a request, Requests cannot release the connection back to the pool unless you consume all the data or call Response.close. This can lead to inefficiency with connections.

In our case, with combination with Artifactory, these open connections are stuck in the middle of a download, because Artifactory does not support the Range requests for lazy wheel.

Not sure if I need to write test for this, let me know if so.

@Tasssadar
Copy link
Contributor Author

Tasssadar commented Dec 13, 2024

I just found that this does not really matter if we were using just requests, because finalizers would take care of closing the responses - but there is also a bug in cachecontrol, which creates circular references and prevents the finalizers from working: psf/cachecontrol#344

We should still be closing the responses here though, as that is what the docs say.

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Good catch. 👍

Not sure if I need to write test for this, let me know if so.

Do you think a test makes sense? If the only way to test it, is to assert that close() is called, it may not be worth it.

src/poetry/inspection/lazy_wheel.py Show resolved Hide resolved
@Tasssadar
Copy link
Contributor Author

Good catch. 👍

Not sure if I need to write test for this, let me know if so.

Do you think a test makes sense? If the only way to test it, is to assert that close() is called, it may not be worth it.

Fixed the exception type. In my opinion, the test do not make much sense, as they would be pretty fragile and would not check any new places where requests usage could be added in the future.

@radoering radoering merged commit 625f42e into python-poetry:main Dec 16, 2024
73 checks passed
@racinmat
Copy link

Hi, when can we expect this being released to pypi?

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.

3 participants