Forkserver singleton #21
Conversation
goodboy
force-pushed
the
forkserver_singleton
branch
2 times, most recently
from
caa232c
to
47c2e47
Compare
| ) | ||
| from multiprocessing.forkserver import ( | ||
| ForkServer, MAXFDS_TO_SEND, write_unsigned, | ||
| # _serve_one, |
There was a problem hiding this comment.
Can this be relied on in 3.6?
tractor/_forkserver_hackzorz.py
Outdated
| SIGNED_STRUCT = struct.Struct('q') # large enough for pid_t | ||
|
|
||
|
|
||
| class AdultForkServer(ForkServer): |
There was a problem hiding this comment.
Maybe SingletonForkServer is a better name?
| # Incoming fork request | ||
| with listener.accept()[0] as s: | ||
|
|
||
| # Thing changed - be tolerant of socket disconnects |
| semaphore_tracker.getfd()] | ||
| allfds += fds | ||
|
|
||
| # This is the only part changed |
| The calling process should write to data_w the pickled preparation and | ||
| process data. | ||
| ''' | ||
| # self.ensure_running() # treat our users like adults expecting |
There was a problem hiding this comment.
Expect client code to call this manually, once, at server startup.
tractor/_trionics.py
Outdated
| @@ -26,6 +28,7 @@ def __init__(self, actor, supervisor=None): | |||
| # self.cancel_scope = cancel_scope | |||
| self._children = {} | |||
| self.cancelled = False | |||
| self._fs = None | |||
There was a problem hiding this comment.
Maybe _forkserver is better.
| ''' | ||
| # self.ensure_running() # treat our users like adults expecting | ||
| # them to spawn the server on their own | ||
| if len(fds) + 4 >= MAXFDS_TO_SEND: |
There was a problem hiding this comment.
As an aside, what's the fourth descriptor?
There was a problem hiding this comment.
No idea - ain't my code. There's 2 pipes and a socket iirc.
tractor/_forkserver_hackzorz.py
Outdated
| reduction.sendfds(client, allfds) | ||
| break | ||
| except OSError as err: | ||
| if err.args[0] == 9: |
There was a problem hiding this comment.
Really should use errno.EBADF instead of 9. Also errno attribute maps to args[0] - imho clearer:
import errno
...
except OSError as err:
if err.errno = errno.EBADF
tractor/_forkserver_hackzorz.py
Outdated
| SIGNED_STRUCT = struct.Struct('q') # large enough for pid_t | ||
|
|
||
|
|
||
| class AdultForkServer(ForkServer): |
tractor/_forkserver_hackzorz.py
Outdated
| print(f"Bad FD {err}") | ||
| client = socket.socket(socket.AF_UNIX) | ||
| client.connect(self._forkserver_address) | ||
| continue |
There was a problem hiding this comment.
How many times does this hit? Might be interesting doing an os.walk on `/proc/self/fd/'
There was a problem hiding this comment.
iirc a couple times per proc. Yeah definitely worth further investigation.
Fwiw it seems forkserver.py is under pretty active development in cpython 3.8.
| fs._forkserver_pid, | ||
| semaphore_tracker._semaphore_tracker._pid, | ||
| semaphore_tracker._semaphore_tracker._fd, | ||
| ) = self._actor._forkserver_info |
There was a problem hiding this comment.
These two assignments are kinda tricky: you're building a tuple, unpacking tuples two different ways, while also chaining assignment. Its clever, but maybe borderline not readable?
At the very least, I could see someone assuming these two statements as similar - as they're written to look parallel - but they're not. The dominating block of the top constructs a tuple, while its unpacking one in the bottom. That last assignment could be visually easy to overlook.
Personal rule of thumb (imho) - the further you get to the end of a line, the less important the details should be.
I'd consider rewriting this, even if it makes it more verbose - but I don't necessarily consider this an egregiously offense either.
goodboy
force-pushed
the
forkserver_singleton
branch
from
cd17e29
to
36400ea
Compare
goodboy
force-pushed
the
forkserver_singleton
branch
from
fe276f9
to
11cc355
Compare
The stdlib insists on creating multiple forkservers and semaphore trackers for each sub-sub-process launched. This isn't ideal since it costs each `tractor` sub-actor an additional 2 more processes then necessary and is confusing when viewed as a process tree (eg. via `pstree`). The majority of the change is simply avoiding the call to `forkserver.ensure_running()` and `semaphore_tracker.ensure_running()` in `ForkServer.connect_new_process()` and instead treating the user like an adult and expecting those calls to be made *once* in the parent most process (i.e. what `multiprocessing` calls the `MainProcess`). Really a proper patch should be made against cpython which allows for similar manual management of the server along with a mechanism to communicate forkserver and semaphore tracker fd info to sub-processes such that further calls to `Process.start()` work as expected. Relates to #6
Start a forkserver once in the main (parent-most) process and pass ipc info (fds) to subprocesses manually such that embedded calls to `multiprocessing.Process.start()` just work. Note that this relies on our overridden version of the stdlib's `multiprocessing.forkserver` module. Resolves #6
goodboy
force-pushed
the
forkserver_singleton
branch
from
11cc355
to
e7c7391
Compare
Temporary solution for #6 by overriding some stdlib code in
multiprocessing.forkserver.py.A proper patch should be made eventually to get this more granular server management into the stdlib. For more detailed discussion see #20.
@vodik ping ping!