-
Notifications
You must be signed in to change notification settings - Fork 167
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
feat: add p2p daemon #164
feat: add p2p daemon #164
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from hivemind.p2p.p2p_daemon import P2P |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import subprocess | ||
import typing as tp | ||
|
||
|
||
class P2P(object): | ||
""" | ||
Forks a child process and executes p2pd command with given arguments. | ||
Sends SIGKILL to the child in destructor and on exit from contextmanager. | ||
""" | ||
|
||
LIBP2P_CMD = 'p2pd' | ||
|
||
def __init__(self, *args, **kwargs): | ||
self._child = subprocess.Popen(args=self._make_process_args(args, kwargs)) | ||
try: | ||
stdout, stderr = self._child.communicate(timeout=0.2) | ||
except subprocess.TimeoutExpired: | ||
pass | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will do for now, but the waiting time may become an issue.
After we add client interaction with p2pd, i'd recommend ping-ing P2PD with the client, and check for errors if that fails. |
||
else: | ||
raise RuntimeError(f'p2p daemon exited with stderr: {stderr}') | ||
|
||
def __enter__(self): | ||
return self._child | ||
|
||
def __exit__(self, exc_type, exc_val, exc_tb): | ||
self._kill_child() | ||
|
||
def __del__(self): | ||
self._kill_child() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Nitpicking] This will handle del/dereferencing, but not sigkill. Is there perhaps a way to create a non-unix-daemon (aka python daemon) that is guaranteed to die with the host process? |
||
|
||
def _kill_child(self): | ||
if self._child.poll() is None: | ||
self._child.kill() | ||
self._child.wait() | ||
|
||
def _make_process_args(self, args: tp.Tuple[tp.Any], | ||
kwargs: tp.Dict[str, tp.Any]) -> tp.List[str]: | ||
proc_args = [self.LIBP2P_CMD] | ||
proc_args.extend( | ||
str(entry) for entry in args | ||
) | ||
proc_args.extend( | ||
f'-{key}={str(value)}' for key, value in kwargs.items() | ||
) | ||
return proc_args |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
import subprocess | ||
from time import perf_counter | ||
|
||
import pytest | ||
|
||
import hivemind.p2p | ||
from hivemind.p2p import P2P | ||
|
||
RUNNING = 'running' | ||
NOT_RUNNING = 'not running' | ||
CHECK_PID_CMD = ''' | ||
if ps -p {0} > /dev/null; | ||
then | ||
echo "{1}" | ||
else | ||
echo "{2}" | ||
fi | ||
''' | ||
|
||
|
||
def is_process_running(pid: int) -> bool: | ||
cmd = CHECK_PID_CMD.format(pid, RUNNING, NOT_RUNNING) | ||
return subprocess.check_output(cmd, shell=True).decode('utf-8').strip() == RUNNING | ||
|
||
|
||
@pytest.fixture() | ||
def mock_p2p_class(): | ||
P2P.LIBP2P_CMD = "sleep" | ||
|
||
|
||
def test_daemon_killed_on_del(mock_p2p_class): | ||
start = perf_counter() | ||
p2p_daemon = P2P('10s') | ||
|
||
child_pid = p2p_daemon._child.pid | ||
assert is_process_running(child_pid) | ||
|
||
del p2p_daemon | ||
assert not is_process_running(child_pid) | ||
assert perf_counter() - start < 1 | ||
|
||
|
||
def test_daemon_killed_on_exit(mock_p2p_class): | ||
start = perf_counter() | ||
with P2P('10s') as daemon: | ||
child_pid = daemon.pid | ||
assert is_process_running(child_pid) | ||
|
||
assert not is_process_running(child_pid) | ||
assert perf_counter() - start < 1 | ||
|
||
|
||
def test_daemon_raises_on_faulty_args(): | ||
with pytest.raises(RuntimeError): | ||
P2P(faulty='argument') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deniskamazur sanity check: can we guarantee that the p2pd you compile in #153 will be always accessible as P2Pd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as long as the
main
package stays in the p2pd directory.That's what
go help install
says:Install compiles and installs the packages named by the import paths...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually might be a good idea to download a specific version of libp2p-daemon rather than the latest version of the master branch to make sure refactorings of their codebase don't break our builds.