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

Close unclosed MemoryObjectReceiveStream in TestClient #2693

Merged
merged 1 commit into from
Sep 22, 2024

Conversation

Moataz-E
Copy link
Contributor

Summary

TestClient isn't closing stream_receive: StreamObjectStream correctly during shutdown. This has been confirmed by multiple people as part of discussion #2603. @markwaddle has identified that the issue is due to stream_receive not being disposed of correctly in the shutdown function. Adding stream_receive to the shutdown function's context manager resolved the issue as confirmed by myself and others in the discussion.

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.

Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Kludex
Copy link
Member

Kludex commented Sep 17, 2024

We have an ignore of resource warning on the pyproject. Is this PR enough to remove that?

@abdullahsych
Copy link

abdullahsych commented Sep 21, 2024

@Kludex Any idea when this will be merged? We are currently working around this issue by pinning an older version of anyio.

@Kludex
Copy link
Member

Kludex commented Sep 21, 2024

It would be cool to get my question replied...

@Moataz-E
Copy link
Contributor Author

It would be cool to get my question replied...

I removed the ignore filter and re-ran the tests and can see 7 tests still fail because of this warning. So this only prevents the issue from propagating to people who are using TestClient.

@Kludex If you would like these issues resolved first before the PR is merged, let me know and I can take a look this evening.

@Kludex
Copy link
Member

Kludex commented Sep 21, 2024

It would be cool to get my question replied...

I removed the ignore filter and re-ran the tests and can see 7 tests still fail because of this warning. So this only prevents the issue from propagating to people who are using TestClient.

@Kludex If you would like these issues resolved first before the PR is merged, let me know and I can take a look this evening.

Ideally, it would be cool to solve everything that was introduced by the bump of anyio on the project before merging this.

But... It's not a blocker...

@Moataz-E
Copy link
Contributor Author

It would be cool to get my question replied...

I removed the ignore filter and re-ran the tests and can see 7 tests still fail because of this warning. So this only prevents the issue from propagating to people who are using TestClient.
@Kludex If you would like these issues resolved first before the PR is merged, let me know and I can take a look this evening.

Ideally, it would be cool to solve everything that was introduced by the bump of anyio on the project before merging this.

But... It's not a blocker...

No worries, I will take a look when I get back on my computer.

@Kludex
Copy link
Member

Kludex commented Sep 21, 2024

Thanks.

@Kludex Kludex changed the title Resolve memory leak in TestClient Close unclosed MemoryObjectReceiveStream on TestClient Sep 22, 2024
@Kludex Kludex changed the title Close unclosed MemoryObjectReceiveStream on TestClient Close unclosed MemoryObjectReceiveStream in TestClient Sep 22, 2024
@Kludex
Copy link
Member

Kludex commented Sep 22, 2024

I'll make a release with this PR, but I think this will lower the motivation to find out why we have more unclosed memory objects... 😢

@Kludex Kludex merged commit 6d70ad3 into encode:master Sep 22, 2024
6 checks passed
nixroxursox pushed a commit to nixroxursox/starlette that referenced this pull request Sep 30, 2024
@alvindera97
Copy link

alvindera97 commented Oct 4, 2024

I had more warnings by a factor of about 4X...

Edit:
My mistake. I had to update starlette manually to the latest version (as of time of writing) 0.39.2 and now I don't have any warnings anymore.

@Moataz-E
Copy link
Contributor Author

Moataz-E commented Oct 4, 2024

I had more warnings by a factor of about 4X

Which version were you running before and after? Can you provide more details about the warning numbers.

@alvindera97
Copy link

I had more warnings by a factor of about 4X

Which version were you running before and after? Can you provide more details about the warning numbers.

Updating anyio to the latest version (as of writing) 4.6.0 was what increased the warning by a factor of 4. I was previously on anyio version 4.4.0.

So updating to the latest version starlette got rid of all the warnings.

alvindera97 added a commit to alvindera97/T that referenced this pull request Oct 5, 2024
Resolve Unclosed Memory Stream Object during tests encode/starlette#2693.
alvindera97 added a commit to alvindera97/T that referenced this pull request Oct 5, 2024
Resolve Unclosed Memory Stream Object during tests encode/starlette#2693.
@Moataz-E
Copy link
Contributor Author

Moataz-E commented Oct 5, 2024

@alvindera97 if the issue is related to anyio and not this change then please create a separate issue / discussion.

@juliangilbey
Copy link

I'm still seeing this error when testing fastapi using the following package versions:

  • fastapi version 0.115.5
  • starlette version 0.41.2
  • anyio version 4.6.2
    So there still seem to be cases where this PR is insufficient to close the streams, unfortunately. But I have no idea how to track down the cause.

@Kludex
Copy link
Member

Kludex commented Nov 20, 2024

If you read this PR, I mention this in the beginning.

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.

6 participants