Skip to content

Conversation

@Tasssadar
Copy link
Contributor

@Tasssadar Tasssadar commented Dec 13, 2024

Currently, this library breaks requests behavior, where the finalizer will close response if user forges to call close. Use WeakRefs to prevent this.

Comment on lines -142 to -143
response._update_chunk_length = types.MethodType( # type: ignore[method-assign]
_update_chunk_length, response
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you assign a method like this, you're creating a circular reference to self, because when you normally call response._update_chunk_length, it actually creates the bound method on-the-fly, it's not set there all the time.

This SO response helped me understand it: https://stackoverflow.com/a/26158130

Copy link
Contributor

@frostming frostming 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 and fix. I would like to accept this. And I will wait for the inputs from @woodruffw

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

Thanks, makes sense to me as well. I appreciate the additional unit test!

@frostming frostming merged commit 904ff9a into psf:master Dec 17, 2024
18 checks passed
@racinmat
Copy link

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

@woodruffw
Copy link
Member

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

Assuming nothing else comes up, I'll cut a release in the next day or so.

@racinmat
Copy link

racinmat commented Jan 7, 2025

@woodruffw sorry for bothering, but any update?

@woodruffw
Copy link
Member

Sorry for the delay; I've opened #350 to prep the release.

@woodruffw
Copy link
Member

This has been released with v0.14.2. Thanks for the ping @racinmat!

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.

4 participants