unix-ffi: Fixing behavior to avoid zombie threads. - #1046
Conversation
|
bump up |
|
It would be good to add a test for this functionality, Other tests for the ffi module live in the micropython repo under tests/ports/unix you could take one of the other ffi_ tests as the base, and add example code to it. |
|
|
||
| class _PopenStream: | ||
| def __init__(self, fd, pid, mode): | ||
| self._fd = open(f"/dev/fd/{fd}", mode) |
There was a problem hiding this comment.
This needs to use builtins.open so that the returned object is a stream.
| pid, status = waitpid(self._pid, 0) | ||
| self._exitcode = status | ||
| self._closed = True | ||
| return self._exitcode |
There was a problem hiding this comment.
This _PopenStream object will need to implement read/write/etc operations, so that you can communicate with the process.
Eg prior to this patch it's possible to do:
import os
ls = os.popen("/bin/ls")
print(ls.read())
ls.close()and that still needs to work.
|
I'll update the PR. |
|
@klukonin do you have any time to update this PR based on the feedback? |
|
Yes. Sorry for the delay. |
Previously popen() returned the pipe stream directly and the forked child
was never waited for, so it stayed around as a zombie.
Return a small wrapper object instead. It forwards stream operations to
the pipe, so existing code such as os.popen("/bin/ls").read() keeps
working, and it waits for the child when it is closed, either explicitly
or at the end of a with statement. As in CPython, close() returns None if
the process exited successfully and the exit status otherwise.
The child now replaces itself with the command using execvp() instead of
calling system(), which removes the extra intermediate shell process.
Fixes issue micropython#780.
Signed-off-by: Kirill Lukonin (Evil Wireless Man) <klukonin@gmail.com>
The test covers reading from and writing to a process, use as a context manager, the exit status reported by close(), and that a process which has been closed leaves neither a zombie nor an open file descriptor behind. It is run by CI, which needs the stat package to be installed for the os package to import. Signed-off-by: Kirill Lukonin (Evil Wireless Man) <klukonin@gmail.com>
4 participants
This commit is fixing this issue:
#780
As discussed here https://github.com/orgs/micropython/discussions/13239
Now the pid of child thread will be stored and closed properly. Even with context manager.