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

Python io implementation doesn't flush with write_through=True unlike C implementation #65595

Closed
4kir4 mannequin opened this issue Apr 30, 2014 · 7 comments
Closed

Python io implementation doesn't flush with write_through=True unlike C implementation #65595

4kir4 mannequin opened this issue Apr 30, 2014 · 7 comments
Labels
topic-IO type-bug An unexpected behavior, bug, or error

Comments

@4kir4
Copy link
Mannequin

4kir4 mannequin commented Apr 30, 2014

BPO 21396
Nosy @pitrou, @4kir4, @vadmium
Files
  • io-write_through-c-vs-python-issue21396.patch: make C io implementation behave as documented like _pyio already does, add corresponding tests
  • 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 2014-05-08.22:34:18.270>
    created_at = <Date 2014-04-30.10:36:57.479>
    labels = ['type-bug', 'expert-IO']
    title = "Python io implementation doesn't flush with write_through=True unlike C implementation"
    updated_at = <Date 2014-05-08.22:34:18.268>
    user = 'https://github.com/4kir4'

    bugs.python.org fields:

    activity = <Date 2014-05-08.22:34:18.268>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-05-08.22:34:18.270>
    closer = 'pitrou'
    components = ['IO']
    creation = <Date 2014-04-30.10:36:57.479>
    creator = 'akira'
    dependencies = []
    files = ['35125']
    hgrepos = []
    issue_num = 21396
    keywords = ['patch']
    message_count = 7.0
    messages = ['217600', '217654', '217684', '218129', '218130', '218132', '218133']
    nosy_count = 4.0
    nosy_names = ['pitrou', 'akira', 'python-dev', 'martin.panter']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue21396'
    versions = ['Python 3.4', 'Python 3.5']

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Apr 30, 2014

    related: msg217596 (bufsize=1 is broken if subprocess module uses Python io)

    TextIOWrapper.write behavior:

    _pyio.py 1:

            if self._line_buffering and (haslf or "\r" in s):
                self.flush()

    textio.c 2:

    if (self->write_through)
        needflush = 1;
    else if (self->line_buffering &&
        (haslf ||
         PyUnicode_FindChar(text, '\r', 0, PyUnicode_GET_LENGTH(text), 1) != -1))
        needflush = 1;
    

    C implementation calls flush() if write_through=True, Python implementation doesn't.

    @4kir4 4kir4 mannequin added topic-IO type-bug An unexpected behavior, bug, or error labels Apr 30, 2014
    @pitrou
    Copy link
    Member

    pitrou commented Apr 30, 2014

    Thanks for the report, Akira. Feel free to submit a patch! (with a test)

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented May 1, 2014

    I've uploaded the patch that makes C implementation behave according
    to the docs like Python implementation with the corresponding tests.

    Issue bpo-21332 is a dependency for this issue: subprocess' test_universal_newlines needs to be updated to work with Python version.

    For Reference
    -------------

    issue bpo-12591 introduces write_through to support subprocess' stdin
    pipe in text mode with bufsize=0 i.e., TextIOWrapper.buffer is
    unbuffered (raw) and Python and C implementation behave the same in
    this particular case.

    C implementation (pseudo-code)::

      if self.write_through:
          flush_text_buffer()
          buffer.flush() # <-- undocumented

    Python implementation::

       if self.write_through:
           pass # _pyio.TextIOWrapper.write() calls buffer.write() directly

    behaves according to the current documentation 1:

    If *write_through* is ``True``, calls to :meth:`write` are guaranteed
    not to be buffered: any data written on the :class:`TextIOWrapper`
    object is immediately handled to its underlying binary *buffer*

    For reference, here's how subprocess.py uses write_through 2::

      self.stdin = io.open(pipe, 'wb', bufsize)
      if universal_newlines=True:
          self.stdin = io.TextIOWrapper(self.stdin,
              write_through=True,
              line_buffering=(bufsize == 1)) # <-- issue python/cpython#65531

    http://hg.python.org/cpython/rev/9ce8fa0a0899/ - introduce io.TextIOWrapper in subprocess
    http://hg.python.org/cpython/rev/5cc536fbd7c1 - introduce write_through

    @pitrou
    Copy link
    Member

    pitrou commented May 8, 2014

    The patch looks basically fine. I will make a few tweaks to the comments in the test case.

    @pitrou
    Copy link
    Member

    pitrou commented May 8, 2014

    Actually, with the patch, the universal_newlines tests in test_subprocess hang (quite logically, since they lack a flush()). I will fix them as well.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 8, 2014

    New changeset 39f2a78f4357 by Antoine Pitrou in branch '3.4':
    Issue bpo-21396: Fix TextIOWrapper(..., write_through=True) to not force a flush() on the underlying binary stream.
    http://hg.python.org/cpython/rev/39f2a78f4357

    New changeset 37d0c41ed8ad by Antoine Pitrou in branch 'default':
    Issue bpo-21396: Fix TextIOWrapper(..., write_through=True) to not force a flush() on the underlying binary stream.
    http://hg.python.org/cpython/rev/37d0c41ed8ad

    @pitrou
    Copy link
    Member

    pitrou commented May 8, 2014

    Thank you for the patch!

    @pitrou pitrou closed this as completed May 8, 2014
    @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
    topic-IO type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant