Skip to content

Close unclosed MemoryObjectReceiveStream in TestClient#2693

Merged
Kludex merged 1 commit intoKludex:masterfrom
Moataz-E:master
Sep 22, 2024
Merged

Close unclosed MemoryObjectReceiveStream in TestClient#2693
Kludex merged 1 commit intoKludex:masterfrom
Moataz-E:master

Conversation

@Moataz-E
Copy link
Contributor

@Moataz-E Moataz-E commented Sep 17, 2024

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.

adriangb
adriangb approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@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
Owner

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 •
edited
Loading

@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
Owner

Kludex commented Sep 21, 2024

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

@Moataz-E
Copy link
Contributor Author

Moataz-E 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.

@Kludex
Copy link
Owner

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

Moataz-E 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...

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

@Kludex
Copy link
Owner

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
Owner

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 Kludex:master Sep 22, 2024
@alvindera97
Copy link

alvindera97 commented Oct 4, 2024 •
edited
Loading

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

alvindera97 commented Oct 5, 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.

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
@alvindera97
Update Starlette And FastAPI Versions
86cd80c 
Resolve Unclosed Memory Stream Object during tests Kludex/starlette#2693.
alvindera97 added a commit to alvindera97/T that referenced this pull request Oct 5, 2024
@alvindera97
Update Starlette And FastAPI Versions
9ce4c06 
Resolve Unclosed Memory Stream Object during tests Kludex/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

juliangilbey commented Nov 19, 2024

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
Owner

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

Reviewers

1 more reviewer

@adriangb adriangb adriangb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Moataz-E Moataz-E

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Moataz-E @Kludex @abdullahsych @alvindera97 @juliangilbey @adriangb