[GitHub] [beam] chamikaramj commented on a diff in pull request #22654: Remove subprocess.PIPE usage by using a temp file

[GitBox <gi...@apache.org>]

chamikaramj commented on code in PR #22654:
URL: https://github.com/apache/beam/pull/22654#discussion_r942883972


##########
sdks/python/apache_beam/utils/subprocess_server.py:
##########
@@ -147,6 +157,13 @@ def stop_process(self):
       if self._process.poll() is None:
         self._process.kill()
       self._process = None
+      if self._stdout_file_name and os.path.exists(self._stdout_file_name):
+        try:
+          os.unlink(self._stdout_file_name)
+        except Exception as e:
+          logging.error((
+              'Could not remove temporary file %s due to %r' %
+              (self._stdout_file_name, e)))

Review Comment:
   I think we have to manually delete since we set "delete=False" when creating the temp file.



##########
sdks/python/apache_beam/utils/subprocess_server.py:
##########
@@ -114,17 +114,27 @@ def start_process(self):
         cmd = [arg.replace('{{PORT}}', str(port)) for arg in self._cmd]
       endpoint = 'localhost:%s' % port
       _LOGGER.info("Starting service with %s", str(cmd).replace("',", "'"))
+
+      with tempfile.NamedTemporaryFile(delete=False) as stdout_file:
+        self._stdout_file_name = stdout_file.name

Review Comment:
   I don't think the file  will be deleted since we set "delete=False" but probably safer to not use a context manager here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org