search

amoffat / sh

Python process launching
https://sh.readthedocs.io/en/latest/
MIT License
6.98k stars 506 forks source link

Potential resource leak: `stdout` and `stderr` file descriptors not closed #738

Closed allrob23 closed 3 weeks ago

allrob23 commented 3 weeks ago

I noticed a potential resource leak in the Command class when stdout and stderr are redirected to files. The current implementation does not explicitly close these file descriptors after command execution, which could lead to resource leak and to file descriptor exhaustion if many commands are run without proper resource management. Code lines 1494 to 1507.

https://github.com/amoffat/sh/blob/695922ec1f559fc6cfc0424db5931bd37ed04d87/sh.py#L1494-L1507

I'm unsure of the best approach to handle this. My initial thought is to add .close() calls for stdout and stderr after execution, but I’m concerned about potential issues, especially in multi-threaded scenarios where file descriptors might be shared or scenarios where it is necessary to wait to execute.

Could you provide some guidance on the best way to resolve this? I’d appreciate any suggestions or ideas for making this more robust.

Thank you for your help!

amoffat commented 3 weeks ago

Can you explain more why you are thinking this is a resource leak? Python will handle closing the underlying file when it garbage collects the object.

Is this affecting something you're trying to do?

allrob23 commented 3 weeks ago

Can you explain more why you are thinking this is a resource leak?

This blog, for example, explains potential issues that can happen when files are not properly closed (e.g., data loss, file corruption, etc.). There are other similar documents on the web with similar explanations: https://sqlpad.io/tutorial/why-close-file-python/#:~:text=When%20you%20write%20to%20a,the%20file%2C%20ensuring%20data%20integrity.

Python will handle closing the underlying file when it garbage collects the object.

It’s true that Python will eventually close files when garbage collecting, but relying on this behavior isn’t ideal for proper resource management. The same blog emphasizes that "this should not be relied upon", and that it’s always better to explicitly close files to avoid potential issues.

Is this affecting something you're trying to do?

No, not immediately. However, this would certainly require a higher load on the system.

amoffat commented 3 weeks ago

Thanks for the information.

Using stderr and stdout as a string filename is totally optional, and if someone wants to do that, they accept not having control over the management of those resources. If they want full control, they can open a file and pass the handle into the command.

I appreciate you trying to find potential issues, but in the interest of using this platform for reporting reproducible bugs, I'm going to close this.