Patchwork [1,of,2,stable] worker: do not suppress EINTR

login
register
mail settings
Submitter Manuel Jacob
Date May 25, 2022, 12:30 a.m.
Message ID <d058898bdd462b03c5bf.1653438629@tmp>
Download mbox | patch
Permalink /patch/50990/
State New
Headers show

Comments

Manuel Jacob - May 25, 2022, 12:30 a.m.
# HG changeset patch
# User Manuel Jacob <me@manueljacob.de>
# Date 1653433864 -7200
#      Wed May 25 01:11:04 2022 +0200
# Branch stable
# Node ID d058898bdd462b03c5bff38ad40d1f855ea51c23
# Parent  477b5145e1a02715f846ce017b460858a58e03b1
# EXP-Topic worker-pickle-load-EINTR
worker: do not suppress EINTR

Before this change, when IOError with errno EINTR was raised during
pickle.load(), the error was suppressed and loading from other file descriptors
was continued.

On Python 3, system calls failing with EINTR are retried (PEP 475). Therefore,
the removal of this code should not make any difference.

On Python 2, this is not generally the case. CPickle has no handling of EINTR.
In one place it misinterprets it as EOF. In another place, it will raise
IOError. However, this could happen in the middle of the stream. In this case,
if pickle tries to load from the stream later, it will behave wrongly (usually
it will raise an error, but loading of incorrect data or interpreter crashes
are thinkable).
Raphaël Gomès - May 25, 2022, 9:25 a.m.
Before I review these (and your previous series), is there a reason why 
you use the ML instead of Heptapod (as per 
https://www.mercurial-scm.org/pipermail/mercurial-devel/2022-May/148466.html 
) considering you're already using it for your pre-submission CI?

On 5/25/22 02:30, Manuel Jacob wrote:
> # HG changeset patch
> # User Manuel Jacob <me@manueljacob.de>
> # Date 1653433864 -7200
> #      Wed May 25 01:11:04 2022 +0200
> # Branch stable
> # Node ID d058898bdd462b03c5bff38ad40d1f855ea51c23
> # Parent  477b5145e1a02715f846ce017b460858a58e03b1
> # EXP-Topic worker-pickle-load-EINTR
> worker: do not suppress EINTR
>
> Before this change, when IOError with errno EINTR was raised during
> pickle.load(), the error was suppressed and loading from other file descriptors
> was continued.
>
> On Python 3, system calls failing with EINTR are retried (PEP 475). Therefore,
> the removal of this code should not make any difference.
>
> On Python 2, this is not generally the case. CPickle has no handling of EINTR.
> In one place it misinterprets it as EOF. In another place, it will raise
> IOError. However, this could happen in the middle of the stream. In this case,
> if pickle tries to load from the stream later, it will behave wrongly (usually
> it will raise an error, but loading of incorrect data or interpreter crashes
> are thinkable).
>
> diff --git a/mercurial/worker.py b/mercurial/worker.py
> --- a/mercurial/worker.py
> +++ b/mercurial/worker.py
> @@ -301,10 +301,6 @@
>                       selector.unregister(key.fileobj)
>                       key.fileobj.close()
>                       openpipes -= 1
> -                except IOError as e:
> -                    if e.errno == errno.EINTR:
> -                        continue
> -                    raise
>       except:  # re-raises
>           killworkers()
>           cleanup()
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - May 25, 2022, 12:55 p.m.
On Wed, 25 May 2022 02:30:29 +0200, Manuel Jacob wrote:
> # HG changeset patch
> # User Manuel Jacob <me@manueljacob.de>
> # Date 1653433864 -7200
> #      Wed May 25 01:11:04 2022 +0200
> # Branch stable
> # Node ID d058898bdd462b03c5bff38ad40d1f855ea51c23
> # Parent  477b5145e1a02715f846ce017b460858a58e03b1
> # EXP-Topic worker-pickle-load-EINTR
> worker: do not suppress EINTR
> 
> Before this change, when IOError with errno EINTR was raised during
> pickle.load(), the error was suppressed and loading from other file descriptors
> was continued.
> 
> On Python 3, system calls failing with EINTR are retried (PEP 475). Therefore,
> the removal of this code should not make any difference.
> 
> On Python 2, this is not generally the case. CPickle has no handling of EINTR.
> In one place it misinterprets it as EOF. In another place, it will raise
> IOError. However, this could happen in the middle of the stream. In this case,
> if pickle tries to load from the stream later, it will behave wrongly (usually
> it will raise an error, but loading of incorrect data or interpreter crashes
> are thinkable).

Do we want this (and the partial write fix) on stable?

The problem could occur if the payload were quite large, but I think it's
unlikely. Since writer issues a single os.write() in the original code,
reader wouldn't be interrupted in the middle of pickle message in general.

We don't have to think about Python 2 on default, so the patches will get
simplified.
Manuel Jacob - May 25, 2022, 1:29 p.m.
On 25/05/2022 14.55, Yuya Nishihara wrote:
> On Wed, 25 May 2022 02:30:29 +0200, Manuel Jacob wrote:
>> # HG changeset patch
>> # User Manuel Jacob <me@manueljacob.de>
>> # Date 1653433864 -7200
>> #      Wed May 25 01:11:04 2022 +0200
>> # Branch stable
>> # Node ID d058898bdd462b03c5bff38ad40d1f855ea51c23
>> # Parent  477b5145e1a02715f846ce017b460858a58e03b1
>> # EXP-Topic worker-pickle-load-EINTR
>> worker: do not suppress EINTR
>>
>> Before this change, when IOError with errno EINTR was raised during
>> pickle.load(), the error was suppressed and loading from other file descriptors
>> was continued.
>>
>> On Python 3, system calls failing with EINTR are retried (PEP 475). Therefore,
>> the removal of this code should not make any difference.
>>
>> On Python 2, this is not generally the case. CPickle has no handling of EINTR.
>> In one place it misinterprets it as EOF. In another place, it will raise
>> IOError. However, this could happen in the middle of the stream. In this case,
>> if pickle tries to load from the stream later, it will behave wrongly (usually
>> it will raise an error, but loading of incorrect data or interpreter crashes
>> are thinkable).
> 
> Do we want this (and the partial write fix) on stable?
> 
> The problem could occur if the payload were quite large, but I think it's
> unlikely. Since writer issues a single os.write() in the original code,
> reader wouldn't be interrupted in the middle of pickle message in general.

Yes, the problems fixed by the two series exist, but we can assume that 
they are very unlikely (at least nobody complained so far).

I would be fine either way.

> We don't have to think about Python 2 on default, so the patches will get
> simplified.

This patch series is essentially no-op on default, except for removing 
the unnecessary (basically dead code) EINTR handling.

Patch

diff --git a/mercurial/worker.py b/mercurial/worker.py
--- a/mercurial/worker.py
+++ b/mercurial/worker.py
@@ -301,10 +301,6 @@ 
                     selector.unregister(key.fileobj)
                     key.fileobj.close()
                     openpipes -= 1
-                except IOError as e:
-                    if e.errno == errno.EINTR:
-                        continue
-                    raise
     except:  # re-raises
         killworkers()
         cleanup()