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 'ResourceWarning's about unclosed 'anyio.streams.memory.MemoryObjectReceiveStream' #2777

Closed

Conversation

nikitagashkov
Copy link

@nikitagashkov nikitagashkov commented Nov 30, 2024

Fixes #2772.

Summary

This PR addresses the cleanup of memory object streams, particularly focusing on ensuring that streams are properly closed to prevent resource warnings:

  1. Close recv_stream in BaseHTTPMiddleware upon exception within the route to prevent dangling streams.

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.

↑ Looks like no changes in docs needed, this is just a bugfix.

@nikitagashkov nikitagashkov marked this pull request as ready for review November 30, 2024 17:09
@nikitagashkov nikitagashkov force-pushed the fix/anyio-resource-warning branch from 0203498 to 241c4bc Compare November 30, 2024 18:44
Comment on lines 214 to 216
# BaseHTTPMiddleware creates a TaskGroup which copies the context
# and erases any changes to it made within the TaskGroup
assert ctxvar.get() == "set by middleware"
Copy link
Member

Choose a reason for hiding this comment

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

Well, yeah, that's why the pytest.mark.xfail is there... 🤔

Copy link
Author

Choose a reason for hiding this comment

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

xfail complicates testing for a warning, which is why I had to make adjustments.

An AssertionError within the middleware triggers the same ResourceWarning (refer to issue #2778). However, since the warning is emitted only after the stream is deleted, it appears some time later, specifically after an explicit gc.collect() in test_anyio_streams_cleanup_exc_in_route. This causes the test to fail due to a leftover stream
from another test.

Perhaps there's no need for this test at all, as it is currently quite brittle. 🤷‍♂️ It might be better to stop ignoring the warning from anyio once all streams are properly closed.

@Kludex
Copy link
Member

Kludex commented Nov 30, 2024

I'm very confused about the comments in this PR. I'll review the other PR.

Adding tons of comments in unrelated code is not helpful.

@nikitagashkov
Copy link
Author

I'm very confused about the comments in this PR. I'll review the other PR.

Adding tons of comments in unrelated code is not helpful.

I believe comments are useful for highlighting certain parts of the code, but I have removed them as requested.

Additionally, I made the patchset smaller by removing fixes for TestClient warnings and un-ignore of anyio's ResourceWarning, since the original discussion focused on a specific issue. It's probably better to keep these changes separate as well.

@Kludex
Copy link
Member

Kludex commented Dec 3, 2024

Let's continue on the other PR.

@Kludex Kludex closed this Dec 3, 2024
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.

2 participants