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

Consider explicit deletion of stream objects on exit #8

Open
bskinn opened this issue Mar 27, 2018 · 5 comments
Open

Consider explicit deletion of stream objects on exit #8

bskinn opened this issue Mar 27, 2018 · 5 comments
Milestone

Comments

@bskinn
Copy link
Owner

bskinn commented Mar 27, 2018

Depends on the balance between the number of times stdio_mgr is used, the frequency of calls, and the frequency of suitable garbage collection.

Prompted by this SO answer. That example uses a class for the context manager, though, with a recommendation to retain the instance for storing the captured stream contents. With stdio_mgr being implemented as a function, especially one where the streams are closed on context exit, there may not be as big of a problem with memory consumption.

@bskinn bskinn added the maybe This may or may not be implemented label Mar 27, 2018
@bskinn bskinn added this to the Future milestone Mar 27, 2018
@bskinn bskinn added enhancement New feature or request maybe This may or may not be implemented and removed maybe This may or may not be implemented enhancement New feature or request labels Jul 29, 2019
@jayvdb
Copy link
Contributor

jayvdb commented Aug 3, 2019

#3 is sort of related, and on its way to being solved.

I think .close() would have similar effect on memory consumption, as all buffers are released, but worth checking.

@bskinn
Copy link
Owner Author

bskinn commented Aug 3, 2019

In most modern contexts, for anything but the most ridiculous of large streams, I can't imagine this Issue is likely to be a big issue. Seems very YAGNI.

Especially with the changes from #25 and #26, though -- as you noted at the bottom of #26, the mocked stream objects are now storing internal references to the underlying buffers in some situations, and that could lead to memory accumulation if enough things stay in scope/have references.

@bskinn
Copy link
Owner Author

bskinn commented Aug 3, 2019

Although -- both RandomTextIO and TeeStdin store a reference to the buffer in ._buf... as long as the (i, o, e) objects remain in scope, I would think they will retain references to their ._bufs the whole time, even after .close() or .detach(), right?

I guess .close() frees the actual buffer memory, so in that case it is just ._closed_buf to worry about in terms of memory consumption.

But for .detach(), I'd think it would still matter -- though, if someone is .detach()-ing, they likely still want to work with the stream data, and so it would probably be a total footgun to wipe the buffer after just a .detach().

So, yeah: just ._closed_buf to worry about, then.

@bskinn bskinn added wontfix This will not be worked on and removed maybe This may or may not be implemented labels Sep 4, 2019
@bskinn
Copy link
Owner Author

bskinn commented Sep 4, 2019

With #58 switching the persisted-content mechanism from ._closed_buf to a callback, this issue should be moot.

@bskinn bskinn closed this as completed Sep 4, 2019
@jayvdb
Copy link
Contributor

jayvdb commented Sep 8, 2019

I think we at least need tests for this, and it turns out to be very important to delete objects when using TemporaryFile and when using the real sys handles.

@jayvdb jayvdb reopened this Sep 8, 2019
@bskinn bskinn removed the wontfix This will not be worked on label Nov 27, 2022
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

No branches or pull requests

2 participants