Patchwork chgserver: remove Python 2 file descriptor logic

login
register
mail settings
Submitter Yuya Nishihara
Date March 4, 2022, 2:12 a.m.
Message ID <3a9729bead90ef7caaf4.1646359929@lemosa>
Download mbox | patch
Permalink /patch/50679/
State New
Headers show

Comments

Yuya Nishihara - March 4, 2022, 2:12 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1646357326 -32400
#      Fri Mar 04 10:28:46 2022 +0900
# Node ID 3a9729bead90ef7caaf4d25138680c83a888be78
# Parent  46b3ecfb16e2781ede9752d972dc22f0e1dfea87
chgserver: remove Python 2 file descriptor logic

Follows up 0bb28b7736bc "chgserver: remove Python 2 support code."

On Python 2, we had to close newfp prior to restoring the original file
description since "delete newfp" would otherwise close the file descriptor
shared with the long-lived fp:

  in attachio():
    newfp = os.fdopen(fp.fileno(), mode, bufsize)
  in _restoreio():
    newfp.close()  # temporarily close newfp.fileno() (= fp.fileno())
    os.dup2(fd, fp.fileno())  # reopen fp.fileno() with original fd

On the other hand, we shouldn't call newfp.close() on Python 3 since
any function calls are proxied to the underlying file object by
procutil.LineBufferedWrapper.
Raphaël Gomès - March 9, 2022, 10 a.m.
Queued, thanks!

On 3/4/22 03:12, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1646357326 -32400
> #      Fri Mar 04 10:28:46 2022 +0900
> # Node ID 3a9729bead90ef7caaf4d25138680c83a888be78
> # Parent  46b3ecfb16e2781ede9752d972dc22f0e1dfea87
> chgserver: remove Python 2 file descriptor logic
>
> Follows up 0bb28b7736bc "chgserver: remove Python 2 support code."
>
> On Python 2, we had to close newfp prior to restoring the original file
> description since "delete newfp" would otherwise close the file descriptor
> shared with the long-lived fp:
>
>    in attachio():
>      newfp = os.fdopen(fp.fileno(), mode, bufsize)
>    in _restoreio():
>      newfp.close()  # temporarily close newfp.fileno() (= fp.fileno())
>      os.dup2(fd, fp.fileno())  # reopen fp.fileno() with original fd
>
> On the other hand, we shouldn't call newfp.close() on Python 3 since
> any function calls are proxied to the underlying file object by
> procutil.LineBufferedWrapper.
>
> diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
> --- a/mercurial/chgserver.py
> +++ b/mercurial/chgserver.py
> @@ -438,14 +438,8 @@ class chgcmdserver(commandserver.server)
>           nullfd = os.open(os.devnull, os.O_WRONLY)
>           ui = self.ui
>           for (ch, fp, fd), (cn, fn, mode) in zip(self._oldios, _iochannels):
> -            newfp = getattr(ui, fn)
> -            # On Python 3, newfp is just a wrapper around fp even if newfp is
> -            # not fp, so deleting newfp is safe.
> -            if newfp is not fp:
> -                newfp.close()
> -            # restore original fd: fp is open again
>               try:
> -                if newfp is fp and 'w' in mode:
> +                if 'w' in mode:
>                       # Discard buffered data which couldn't be flushed because
>                       # of EPIPE. The data should belong to the current session
>                       # and should never persist.
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Gregory Szorc - March 10, 2022, 2:13 a.m.
I installed @ and am now seeing the following with `alias hg=chg`:

$ hg version
Mercurial Distributed SCM (version 6.1+hg145.469b9ee336a6)
(see https://mercurial-scm.org for more information)

Copyright (C) 2005-2022 Olivia Mackall and others
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
gps@ubuntu-vm-main:~/src/hg (@)$ Traceback (most recent call last):
  File "/home/gps/lib/python/mercurial/commandserver.py", line 509, in
_serverequest
    sv.cleanup()
  File "/home/gps/lib/python/mercurial/chgserver.py", line 382, in cleanup
    self._restoreio()
  File "/home/gps/lib/python/mercurial/chgserver.py", line 454, in
_restoreio
    os.dup2(fd, fp.fileno())
ValueError: I/O operation on closed file
Traceback (most recent call last):
  File "/home/gps/lib/python/mercurial/commandserver.py", line 693, in
_acceptnewconnection
    self._runworker(conn)
  File "/home/gps/lib/python/mercurial/commandserver.py", line 739, in
_runworker
    _serverequest(
  File "/home/gps/lib/python/mercurial/commandserver.py", line 509, in
_serverequest
    sv.cleanup()
  File "/home/gps/lib/python/mercurial/chgserver.py", line 382, in cleanup
    self._restoreio()
  File "/home/gps/lib/python/mercurial/chgserver.py", line 454, in
_restoreio
    os.dup2(fd, fp.fileno())
ValueError: I/O operation on closed file

I didn't bisect. So the regression could be another patch. But it certainly
looks like chg isn't working properly on @.

On Wed, Mar 9, 2022 at 3:02 AM Raphaël Gomès <raphael.gomes@octobus.net>
wrote:

> Queued, thanks!
>
> On 3/4/22 03:12, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1646357326 -32400
> > #      Fri Mar 04 10:28:46 2022 +0900
> > # Node ID 3a9729bead90ef7caaf4d25138680c83a888be78
> > # Parent  46b3ecfb16e2781ede9752d972dc22f0e1dfea87
> > chgserver: remove Python 2 file descriptor logic
> >
> > Follows up 0bb28b7736bc "chgserver: remove Python 2 support code."
> >
> > On Python 2, we had to close newfp prior to restoring the original file
> > description since "delete newfp" would otherwise close the file
> descriptor
> > shared with the long-lived fp:
> >
> >    in attachio():
> >      newfp = os.fdopen(fp.fileno(), mode, bufsize)
> >    in _restoreio():
> >      newfp.close()  # temporarily close newfp.fileno() (= fp.fileno())
> >      os.dup2(fd, fp.fileno())  # reopen fp.fileno() with original fd
> >
> > On the other hand, we shouldn't call newfp.close() on Python 3 since
> > any function calls are proxied to the underlying file object by
> > procutil.LineBufferedWrapper.
> >
> > diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
> > --- a/mercurial/chgserver.py
> > +++ b/mercurial/chgserver.py
> > @@ -438,14 +438,8 @@ class chgcmdserver(commandserver.server)
> >           nullfd = os.open(os.devnull, os.O_WRONLY)
> >           ui = self.ui
> >           for (ch, fp, fd), (cn, fn, mode) in zip(self._oldios,
> _iochannels):
> > -            newfp = getattr(ui, fn)
> > -            # On Python 3, newfp is just a wrapper around fp even if
> newfp is
> > -            # not fp, so deleting newfp is safe.
> > -            if newfp is not fp:
> > -                newfp.close()
> > -            # restore original fd: fp is open again
> >               try:
> > -                if newfp is fp and 'w' in mode:
> > +                if 'w' in mode:
> >                       # Discard buffered data which couldn't be flushed
> because
> >                       # of EPIPE. The data should belong to the current
> session
> >                       # and should never persist.
> >
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Gregory Szorc - March 10, 2022, 2:27 a.m.
Apparently I installed an older @, which wasn't working.

After applying this patch, I _think_ chg is happy again.

On Wed, Mar 9, 2022 at 7:13 PM Gregory Szorc <gregory.szorc@gmail.com>
wrote:

> I installed @ and am now seeing the following with `alias hg=chg`:
>
> $ hg version
> Mercurial Distributed SCM (version 6.1+hg145.469b9ee336a6)
> (see https://mercurial-scm.org for more information)
>
> Copyright (C) 2005-2022 Olivia Mackall and others
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> gps@ubuntu-vm-main:~/src/hg (@)$ Traceback (most recent call last):
>   File "/home/gps/lib/python/mercurial/commandserver.py", line 509, in
> _serverequest
>     sv.cleanup()
>   File "/home/gps/lib/python/mercurial/chgserver.py", line 382, in cleanup
>     self._restoreio()
>   File "/home/gps/lib/python/mercurial/chgserver.py", line 454, in
> _restoreio
>     os.dup2(fd, fp.fileno())
> ValueError: I/O operation on closed file
> Traceback (most recent call last):
>   File "/home/gps/lib/python/mercurial/commandserver.py", line 693, in
> _acceptnewconnection
>     self._runworker(conn)
>   File "/home/gps/lib/python/mercurial/commandserver.py", line 739, in
> _runworker
>     _serverequest(
>   File "/home/gps/lib/python/mercurial/commandserver.py", line 509, in
> _serverequest
>     sv.cleanup()
>   File "/home/gps/lib/python/mercurial/chgserver.py", line 382, in cleanup
>     self._restoreio()
>   File "/home/gps/lib/python/mercurial/chgserver.py", line 454, in
> _restoreio
>     os.dup2(fd, fp.fileno())
> ValueError: I/O operation on closed file
>
> I didn't bisect. So the regression could be another patch. But it
> certainly looks like chg isn't working properly on @.
>
> On Wed, Mar 9, 2022 at 3:02 AM Raphaël Gomès <raphael.gomes@octobus.net>
> wrote:
>
>> Queued, thanks!
>>
>> On 3/4/22 03:12, Yuya Nishihara wrote:
>> > # HG changeset patch
>> > # User Yuya Nishihara <yuya@tcha.org>
>> > # Date 1646357326 -32400
>> > #      Fri Mar 04 10:28:46 2022 +0900
>> > # Node ID 3a9729bead90ef7caaf4d25138680c83a888be78
>> > # Parent  46b3ecfb16e2781ede9752d972dc22f0e1dfea87
>> > chgserver: remove Python 2 file descriptor logic
>> >
>> > Follows up 0bb28b7736bc "chgserver: remove Python 2 support code."
>> >
>> > On Python 2, we had to close newfp prior to restoring the original file
>> > description since "delete newfp" would otherwise close the file
>> descriptor
>> > shared with the long-lived fp:
>> >
>> >    in attachio():
>> >      newfp = os.fdopen(fp.fileno(), mode, bufsize)
>> >    in _restoreio():
>> >      newfp.close()  # temporarily close newfp.fileno() (= fp.fileno())
>> >      os.dup2(fd, fp.fileno())  # reopen fp.fileno() with original fd
>> >
>> > On the other hand, we shouldn't call newfp.close() on Python 3 since
>> > any function calls are proxied to the underlying file object by
>> > procutil.LineBufferedWrapper.
>> >
>> > diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
>> > --- a/mercurial/chgserver.py
>> > +++ b/mercurial/chgserver.py
>> > @@ -438,14 +438,8 @@ class chgcmdserver(commandserver.server)
>> >           nullfd = os.open(os.devnull, os.O_WRONLY)
>> >           ui = self.ui
>> >           for (ch, fp, fd), (cn, fn, mode) in zip(self._oldios,
>> _iochannels):
>> > -            newfp = getattr(ui, fn)
>> > -            # On Python 3, newfp is just a wrapper around fp even if
>> newfp is
>> > -            # not fp, so deleting newfp is safe.
>> > -            if newfp is not fp:
>> > -                newfp.close()
>> > -            # restore original fd: fp is open again
>> >               try:
>> > -                if newfp is fp and 'w' in mode:
>> > +                if 'w' in mode:
>> >                       # Discard buffered data which couldn't be flushed
>> because
>> >                       # of EPIPE. The data should belong to the current
>> session
>> >                       # and should never persist.
>> >
>> > _______________________________________________
>> > Mercurial-devel mailing list
>> > Mercurial-devel@mercurial-scm.org
>> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>

Patch

diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
--- a/mercurial/chgserver.py
+++ b/mercurial/chgserver.py
@@ -438,14 +438,8 @@  class chgcmdserver(commandserver.server)
         nullfd = os.open(os.devnull, os.O_WRONLY)
         ui = self.ui
         for (ch, fp, fd), (cn, fn, mode) in zip(self._oldios, _iochannels):
-            newfp = getattr(ui, fn)
-            # On Python 3, newfp is just a wrapper around fp even if newfp is
-            # not fp, so deleting newfp is safe.
-            if newfp is not fp:
-                newfp.close()
-            # restore original fd: fp is open again
             try:
-                if newfp is fp and 'w' in mode:
+                if 'w' in mode:
                     # Discard buffered data which couldn't be flushed because
                     # of EPIPE. The data should belong to the current session
                     # and should never persist.