revng / revng-orchestra

rev.ng's package manager
4 stars 6 forks source link

orc shell: exec rather than invoking subprocess #59

Closed mrjackv closed 2 years ago

mrjackv commented 2 years ago

This commit does 2 things:

I've also tweaked the typing annotations in the affected functions.

aleclearmind commented 2 years ago

Given our recent experience with temporary files and exec, I wonder if this is a good idea. Here they suggest to fork, let the forked process shutdown and execve in the old process. What do you think?

mrjackv commented 2 years ago

The basic problem is that, without adding a lot of extra logic, the best way to do orc shell is to exec, double-forking would only complicate things and not solve our problem. I checked the codebase and there are only two places where a temporary file is used, and in both cases they're used with a context manager (with), so the only way for a file leak would be to exec within the with (which does not happen since in both occasions we are inside the context for very little time) Also, even if there was an instance of file leakage I'd prefer to fix it (because "morally" the tempfile is extended beyond its actual lifetime) rather handle the alternative fix for orc shell

mrjackv commented 2 years ago

@aleclearmind made exec fork to free resources