[
https://issues.apache.org/jira/browse/CASSANDRA-17350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18112525#comment-18112525
]
Arvind Kandpal commented on CASSANDRA-17350:
--------------------------------------------
Hi, [~smiklosovic] ,
Bowen's concern in CASSANDRA-16956 was that moving the on_fork() code into
_{_}init{_}_ could leak FDs: if an exception happens after _{_}init{_}_ opens
the FDs but before fork() actually runs, nothing on the parent side closes them.
That concern is reason enough on its own not to make the move - so I'm not
proposing that change. Channels stay created in on_fork(), same as today.
While looking into why on_fork() exists in the first place, I found two more
reasons it can't move to _{_}init{_}_, unrelated to Windows:
- CASSANDRA-11053: a generator can't be pickled, so FilesReader.sources has to
be created lazily in start().
- CASSANDRA-11701: the feeding thread was once created lazily and hit a race
between concurrent senders on_fork() exists to create it once,
deterministically, per child process.
- More generally: _{_}init{_}_ runs in the parent, before the process starts.
A thread created there won't survive fork(), and isn't picklable on spawn-based
platforms like macOS.
Also dropped the CASSANDRA-11749 reference in the docstring - that code
(shutting down the inherited parent connection) was already removed in
CASSANDRA-16053.
This patch just cleans up the comments to explain the above, so future readers
don't reopen this question. No behavior change.
> Investigate and remove Windows-specific threading-related code in copyutils.py
> ------------------------------------------------------------------------------
>
> Key: CASSANDRA-17350
> URL: https://issues.apache.org/jira/browse/CASSANDRA-17350
> Project: Apache Cassandra
> Issue Type: Improvement
> Components: Tool/cqlsh
> Reporter: Stefan Miklosovic
> Priority: Normal
> Time Spent: 10m
> Remaining Estimate: 0h
>
> There are bits of the code to be removed or refactored related to how Windows
> were treating threading in copyutils.py as Windows is not longer supported.
> [~Bowen Song] put it best so I just copy it here from GitHub PR for 16956,
> this code relates to FilesReader, FeedingProcess and ChildProcess Python
> classes.
> We agreed on the fact that 16956 may be merged without this being addressed
> as it requires further investigation in the matter which would unncessarily
> postpone and delay it.
> Bowen's take on this:
> 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 message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
