bowensong commented on a change in pull request #1249:
URL: https://github.com/apache/cassandra/pull/1249#discussion_r799733359
##########
File path: pylib/cqlshlib/copyutil.py
##########
@@ -1405,8 +1405,8 @@ def __init__(self, params, target):
super(ChildProcess, self).__init__(target=target)
self.inpipe = params['inpipe']
self.outpipe = params['outpipe']
- self.inmsg = None # must be initialized after fork on Windows
- self.outmsg = None # must be initialized after fork on Windows
+ self.inmsg = None
Review comment:
I believe that we should move the code from `on_fork()` to `__init__()`,
and may also remove the `on_fork()` method if it's no longer used.
The problem with Windows and Python `multiprocessing` is that Windows
doesn't support `fork()`, therefore Python implemented a workaround. On
Windows, Python `multiprocessing` library uses `pickle` to serialise everything
in memory, spawn a new process, and then restores the memory content from the
serialised data. The `ReceivingChannel` and `SendingChannel` objects are not
serialisable because they have file descriptors (which I believe it's called a
file handle on Windows) in them, therefore the code has to handle them after
the fake `fork()`.
However, I'm concerned that moving the code from `on_fork()` to `__init__()`
may have other unintended side effects. For example, the file descriptors (FDs)
will be opened before the fork, in some edge cases the fork may never happen
(e.g.: an exception raised in or just after the init, but before the fork).
Where's the code responsible for closing the FDs on the parent process side?
Will this cause any FD leak? This clearly requires further work to find out.
To be honest, I don't think removing the comments without addressing the
above is a wise move. Future developers wouldn't have the opportunity to
understand why the code is written in this way if the comment is removed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
