-
Notifications
You must be signed in to change notification settings - Fork 310
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 replacing ProcessCapture
with something else
#4668
Comments
Maybe slightly related: Some of the |
The fact that `ProcessCapture` uses temporary files to work around the 64k buffering limit is an implementation detail that should not be exposed to ease switching to another process builder alternative eventually. Also see [1]. [1]: #4668 Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
The fact that `ProcessCapture` uses temporary files to work around the 64k buffering limit is an implementation detail that should not be exposed to ease switching to another process builder alternative eventually. Also see [1]. [1]: #4668 Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
The fact that `ProcessCapture` uses temporary files to work around the 64k buffering limit is an implementation detail that should not be exposed to ease switching to another process builder alternative eventually. Also see [1]. [1]: oss-review-toolkit#4668 Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
The fact that `ProcessCapture` uses temporary files to work around the 64k buffering limit is an implementation detail that should not be exposed to ease switching to another process builder alternative eventually. Also see [1]. [1]: oss-review-toolkit#4668 Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
The fact that `ProcessCapture` uses temporary files to work around the 64k buffering limit is an implementation detail that should not be exposed to ease switching to another process builder alternative eventually. Also see [1]. [1]: oss-review-toolkit#4668 Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
I totally agree that streaming of (parts of) stdout to logs would be very useful! |
Regarding the
getVersion()
invocation mentioned by @mnonnenmacher and @sschuberth, I did some testing. I ran a simple test that executes this method in a loop. What I found out is that there is indeed a small memory leak caused byProcessCapture
callingdeleteOnExit()
on the temporary files it creates to capture the process output.deleteOnExit()
causes the name of the file affected to be recorded in a static Set managed by the internalDeleteOnExitHook
system class, so that it can be deleted later.So each usage of the
ProcessCapture
class adds three file names to this set (for stdout, stderr, and the temporary directory). This could be cured by ensuring manually that the files are cleaned up, e.g. using a try-finally construct. However, my gut feeling is that the amount of memory wasted here is not really significant.Originally posted by @oheger-bosch in #3091 (comment)
The text was updated successfully, but these errors were encountered: