Skip to content
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

multiprocessing.process using os.close(sys.stdin.fileno) instead of sys.stdin.close() #49563

Closed
grahamd mannequin opened this issue Feb 19, 2009 · 12 comments
Closed

multiprocessing.process using os.close(sys.stdin.fileno) instead of sys.stdin.close() #49563

grahamd mannequin opened this issue Feb 19, 2009 · 12 comments

Comments

@grahamd
Copy link
Mannequin

@grahamd grahamd mannequin commented Feb 19, 2009

BPO 5313
Files
  • papy_gui.py: InteractiveConsole inside TkinterText
  • stdin_patch_v2.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2009-06-30.17:13:43.818>
    created_at = <Date 2009-02-19.05:21:10.032>
    labels = []
    title = 'multiprocessing.process using os.close(sys.stdin.fileno) instead of sys.stdin.close()'
    updated_at = <Date 2009-06-30.17:13:43.817>
    user = 'https://bugs.python.org/grahamd'

    bugs.python.org fields:

    activity = <Date 2009-06-30.17:13:43.817>
    actor = 'jnoller'
    assignee = 'jnoller'
    closed = True
    closed_date = <Date 2009-06-30.17:13:43.818>
    closer = 'jnoller'
    components = []
    creation = <Date 2009-02-19.05:21:10.032>
    creator = 'grahamd'
    dependencies = []
    files = ['13704', '14402']
    hgrepos = []
    issue_num = 5313
    keywords = ['patch']
    message_count = 12.0
    messages = ['82457', '82579', '86024', '89195', '89198', '89199', '89205', '89522', '89523', '89529', '89900', '89943']
    nosy_count = 5.0
    nosy_names = ['OG7', 'jnoller', 'grahamd', 'ptn', 'i000']
    pr_nums = []
    priority = 'low'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue5313'
    versions = ['Python 2.6', 'Python 3.0']

    @grahamd
    Copy link
    Mannequin Author

    @grahamd grahamd mannequin commented Feb 19, 2009

    In multiprocessing.process it contains the code:

        def _bootstrap(self):
           ....
                if sys.stdin is not None:
                    try:
                        os.close(sys.stdin.fileno())
                    except (OSError, ValueError):
                        pass

    This code should probably be calling sys.stdin.close() and not
    os.close(sys.stdin.fileno()).

    The code as is will fail if sys.stdin had been replaced with an alternate
    file like object, such as StringIO, which doesn't have a fileno() method.

    @jnoller jnoller mannequin self-assigned this Feb 19, 2009
    @jnoller
    Copy link
    Mannequin

    @jnoller jnoller mannequin commented Feb 21, 2009

    Joshua Judson Rosen to python-list

    Jesse Noller <jnoller@gmail.com> writes:

    On Tue, Feb 17, 2009 at 10:34 PM, Graham Dumpleton
    <Graham.Dumpleton@gmail.com> wrote:
    > Why is the multiprocessing module, ie., multiprocessing/process.py, in
    > _bootstrap() doing:
    >
    > os.close(sys.stdin.fileno())
    >
    > rather than:
    >
    > sys.stdin.close()
    >
    > Technically it is feasible that stdin could have been replaced with
    > something other than a file object, where the replacement doesn't have
    > a fileno() method.
    >
    > In that sort of situation an AttributeError would be raised, which
    > isn't going to be caught as either OSError or ValueError, which is all
    > the code watches out for.

    I don't know why it was implemented that way. File an issue on the
    tracker and assign it to me (jnoller) please.

    My guess would be: because it's also possible for sys.stdin to be a
    file that's open in read+write mode, and for that file to have
    pending output buffered (for example, in the case of a socketfile).

    There's a general guideline, inherited from C, that one should ensure
    that the higher-level close() routine is invoked on a given
    file-descriptor in at most *one* process after that descriptor has
    passed through a fork(); in the other (probably child) processes, the
    lower-level close() routine should be called to avoid a
    double-flush--whereby buffered data is flushed out of one process, and
    then the *same* buffered data is flushed out of the (other)
    child-/parent-process' copy of the file-object.

    So, if you call sys.stdin.close() in the child-process in
    _bootstrap(), then it could lead to a double-flush corrupting output
    somewhere in the application that uses the multiprocessing module.

    You can expect similar issues with just about /any/ file-like objects' that might have file-like semantics' of buffering data and flushing
    it on close, also--because you end up with multiple copies of the same
    object in `pre-flush' state, and each copy tries to flush at some point.

    As such, I'd recommend against just using .close(); you might use
    something like if hasattr(sys.stdin, "fileno"): ...'; but, if your else' clause unconditionally calls sys.stdin.close(), then you still
    have double-flush problems if someone's set sys.stdin to a file-like
    object with output-buffering.

    I guess you could try calling that an edge-case' and seeing if anyone screams. It'd be sort-of nice if there was finer granularity in the file API--maybe if file.close() took a boolean flush' argument....

    @i000
    Copy link
    Mannequin

    @i000 i000 mannequin commented Apr 16, 2009

    Hello,

    I believe I am the edge-case. I've written a minimalist python
    Tkinter-shell around Tkinter.Text and code.InteractiveConsole by
    hi-jacking stdin, stdout and stderr. It "hangs" when using
    multiprocessing pool.

    to reproduce run papy_gui.py
    and type:
    >>> from math import sqrt
    >>> from multiprocessing import Pool
    >>> p = Pool()
    >>> print p
    <multiprocessing.pool.Pool object at 0xb723738c>
    >>> p.map(sqrt, [1,2,3])
    <<no more output ever>>

    @OG7
    Copy link
    Mannequin

    @OG7 OG7 mannequin commented Jun 10, 2009

    Using sys.stdin.close() fixes issues 5155 and 5331.

    @grahamd
    Copy link
    Mannequin Author

    @grahamd grahamd mannequin commented Jun 10, 2009

    Worth noting is that Python documentation in:

    http://docs.python.org/library/stdtypes.html

    says:

    """
    file.fileno()
    Return the integer “file descriptor” that is used by the underlying
    implementation to request I/O operations from the operating system. This
    can be useful for other, lower level interfaces that use file
    descriptors, such as the fcntl module or os.read() and friends.

    Note File-like objects which do not have a real file descriptor should
    not provide this method!
    """

    So, if sys.stdin is replaced with a file like object, then the code
    should not be assuming that fileno() even exists.

    At the moment the code doesn't check for AttributeError which is what is
    going to be raised for trying to access non existent fileno() method.

    """
    >>> import StringIO
    >>> s=StringIO.StringIO("xxx")
    >>> s.fileno()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    AttributeError: StringIO instance has no attribute 'fileno'
    """

    Thus error propagates. The better check though would be to use:

        def _bootstrap(self):
           ....
                if sys.stdin is not None and hasattr(sys.stdin, "fileno"):
                    try:
                        os.close(sys.stdin.fileno())
                    except (OSError, ValueError):
                        pass

    That is, only actually call fileno() if it is present.

    This would solve the problem for the original reported issue by making
    it actually adhere to what Python documentation says about existence of
    fileno().

    This change wouldn't necessarily solve other peoples related issues
    though.

    @ptn
    Copy link
    Mannequin

    @ptn ptn mannequin commented Jun 10, 2009

    Wouldn't it be more pythonic to just try sys.stdin.fileno() and catch
    the AtributeError too?

    def _bootstrap(self):
           ....
                try:
                    os.close(sys.stdin.fileno())
                except AtributeError:
                    sys.stdin.close()
                except (OSError, ValueError):
                    pass

    @OG7
    Copy link
    Mannequin

    @OG7 OG7 mannequin commented Jun 10, 2009

    Closing the stdin fd without closing the stdin file is very dangerous.
    It means that stdin will now access some random resource, for example, a
    pipe created with os.pipe().

    Closing stdin is useful to let the parent be alone in reading from it.
    It can be achieved by replacing stdin by open('/dev/null'). The original
    stdin can be closed or left to be garbage collected.

    The "double flush" case is impossible to handle in general. It's the
    libc's responsibility for standard file objects and sockets. But it
    can't be helped (except by putting a warning in the documentation) if
    someone combines multiprocessing with non-fork-safe code that keeps its
    own buffers and doesn't check for a pid change.

    So that code in _bootstrap should be:
    sys.stdin.close()
    sys.stdin = open(os.devnull)

    @jnoller
    Copy link
    Mannequin

    @jnoller jnoller mannequin commented Jun 19, 2009

    Attached is a patch which calls close() first, and then attempts to close
    the fd. In the case of an attribute errors (fileno doesn't exist) we
    simply set it to devnull.

    This is just a thought, feedback welcome - right now this allows this
    fixes bpo-5155 and 5331 (and this issue)

    @OG7
    Copy link
    Mannequin

    @OG7 OG7 mannequin commented Jun 19, 2009

    Please do this:

    --- a/multiprocessing/process.py
    +++ b/multiprocessing/process.py
    @@ -225,7 +225,8 @@ class Process(object):
                 self._children = set()
                 self._counter = itertools.count(1)
                 try:
    -                os.close(sys.stdin.fileno())
    +                sys.stdin.close()
    +                sys.stdin = open(os.devnull)
                 except (OSError, ValueError):
                     pass
                 _current_process = self

    One shouldn't close the fileno after the file has been closed. The
    stdlib raises an error, and the open(os.devnull) won't be reached. If no
    error was thrown, it would be worse. This would be closing a fileno that
    doesn't belong to sys.stdin anymore and may be used somewhere else.

    The open(os.devnull) bit is both so that no one tries to do anything
    with the old stdin anymore, and to let applications attempt to read from
    stdin without an IOError.

    @jnoller
    Copy link
    Mannequin

    @jnoller jnoller mannequin commented Jun 19, 2009

    On Fri, Jun 19, 2009 at 11:55 AM, OG7<report@bugs.python.org> wrote:

    OG7 <onyxg7@users.sourceforge.net> added the comment:

    One shouldn't close the fileno after the file has been closed. The
    stdlib raises an error, and the open(os.devnull) won't be reached. If no
    error was thrown, it would be worse. This would be closing a fileno that
    doesn't belong to sys.stdin anymore and may be used somewhere else.

    The open(os.devnull) bit is both so that no one tries to do anything
    with the old stdin anymore, and to let applications attempt to read from
    stdin without an IOError.

    Fair enough, I was worried a bit about skipping the
    os.close(sys.stdin.fileno()) - the tests pass with both (because the
    close fixes the basic problem). I need to come up with an appropriate
    documentation note about the double flush issue, add in the tests I
    gleaned from the other bugs and commit it. I'll post the full
    doc/tests/code patch before doing so.

    @jnoller
    Copy link
    Mannequin

    @jnoller jnoller mannequin commented Jun 30, 2009

    Attached is a patch with docs, tests and the fix as suggested by OG7 (whom
    I can't add to the ACKS without a real name). Please check the additional
    doc note I added to the all platforms programming guidelines.

    @jnoller
    Copy link
    Mannequin

    @jnoller jnoller mannequin commented Jun 30, 2009

    Committed in r73708 on trunk

    @jnoller jnoller mannequin closed this as completed Jun 30, 2009
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    No branches or pull requests

    0 participants