Patchwork [STABLE] win32: backout 1a9ebc83a74c

login
register
mail settings
Submitter Adrian Buehlmann
Date May 3, 2014, 8:43 a.m.
Message ID <b3bceb2a103e9f279f05.1399106635@kork>
Download mbox | patch
Permalink /patch/4584/
State Superseded
Commit 4898c37e03c0b804d6ca8c3ad341dceb53ff5806
Headers show

Comments

Adrian Buehlmann - May 3, 2014, 8:43 a.m.
# HG changeset patch
# User Adrian Buehlmann <adrian@cadifra.com>
# Date 1399106034 -7200
# Branch stable
# Node ID b3bceb2a103e9f279f058e7a3bacf272968b6bb6
# Parent  d36440d843284ba546858b241da9cc4817811fe5
win32: backout 1a9ebc83a74c

This appears to cause havoc on TortoiseHg. While 1a9ebc83a74c may be nice to
have (as soon as someone can prove it has no unwanted side effects), it is not
worth the troubles at this point.
Steve Borho - May 5, 2014, 6:56 p.m.
On Sat, May 3, 2014 at 3:43 AM, Adrian Buehlmann <adrian@cadifra.com> wrote:
> # HG changeset patch
> # User Adrian Buehlmann <adrian@cadifra.com>
> # Date 1399106034 -7200
> # Branch stable
> # Node ID b3bceb2a103e9f279f058e7a3bacf272968b6bb6
> # Parent  d36440d843284ba546858b241da9cc4817811fe5
> win32: backout 1a9ebc83a74c
>
> This appears to cause havoc on TortoiseHg. While 1a9ebc83a74c may be nice to
> have (as soon as someone can prove it has no unwanted side effects), it is not
> worth the troubles at this point.

To add some context; the troubles are specifically that files which
are being "watched" for changes by QFileSystemWatcher() are no longer
safely updated by util.rename().  So things like updating .hg/hgrc or
perhaps even the changelog are suddenly dangerous.  We're still not
completely certain of the exact semantics of the problem so I don't
know if this patch is mandatory, but we believe it would resolve the
problem.


> diff --git a/mercurial/win32.py b/mercurial/win32.py
> --- a/mercurial/win32.py
> +++ b/mercurial/win32.py
> @@ -24,7 +24,6 @@
>
>  # GetLastError
>  _ERROR_SUCCESS = 0
> -_ERROR_SHARING_VIOLATION = 32
>  _ERROR_INVALID_PARAMETER = 87
>  _ERROR_INSUFFICIENT_BUFFER = 122
>
> @@ -60,9 +59,7 @@
>
>  _OPEN_EXISTING = 3
>
> -_FILE_FLAG_OPEN_REPARSE_POINT = 0x00200000
>  _FILE_FLAG_BACKUP_SEMANTICS = 0x02000000
> -_FILE_FLAG_DELETE_ON_CLOSE = 0x04000000
>
>  # SetFileAttributes
>  _FILE_ATTRIBUTE_NORMAL = 0x80
> @@ -423,18 +420,11 @@
>  def unlink(f):
>      '''try to implement POSIX' unlink semantics on Windows'''
>
> -    # If we can open f exclusively, no other processes must have open handles
> -    # for it and we can expect its name will be deleted immediately when we
> -    # close the handle unless we have another in the same process.  We also
> -    # expect we shall simply fail to open f if it is a directory.
> -    fh = _kernel32.CreateFileA(f, 0, 0, None, _OPEN_EXISTING,
> -        _FILE_FLAG_OPEN_REPARSE_POINT | _FILE_FLAG_DELETE_ON_CLOSE, None)
> -    if fh != _INVALID_HANDLE_VALUE:
> -        _kernel32.CloseHandle(fh)
> -        return
> -    error = _kernel32.GetLastError()
> -    if error != _ERROR_SHARING_VIOLATION:
> -        raise ctypes.WinError(error)
> +    if os.path.isdir(f):
> +        # use EPERM because it is POSIX prescribed value, even though
> +        # unlink(2) on directories returns EISDIR on Linux
> +        raise IOError(errno.EPERM,
> +                      "Unlinking directory not permitted: '%s'" % f)
>
>      # POSIX allows to unlink and rename open files. Windows has serious
>      # problems with doing that:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - May 5, 2014, 7:37 p.m.
On 05/05/2014 11:56 AM, Steve Borho wrote:
> On Sat, May 3, 2014 at 3:43 AM, Adrian Buehlmann <adrian@cadifra.com> wrote:
>> # HG changeset patch
>> # User Adrian Buehlmann <adrian@cadifra.com>
>> # Date 1399106034 -7200
>> # Branch stable
>> # Node ID b3bceb2a103e9f279f058e7a3bacf272968b6bb6
>> # Parent  d36440d843284ba546858b241da9cc4817811fe5
>> win32: backout 1a9ebc83a74c
>>
>> This appears to cause havoc on TortoiseHg. While 1a9ebc83a74c may be nice to
>> have (as soon as someone can prove it has no unwanted side effects), it is not
>> worth the troubles at this point.
>
> To add some context; the troubles are specifically that files which
> are being "watched" for changes by QFileSystemWatcher() are no longer
> safely updated by util.rename().  So things like updating .hg/hgrc or
> perhaps even the changelog are suddenly dangerous.  We're still not
> completely certain of the exact semantics of the problem so I don't
> know if this patch is mandatory, but we believe it would resolve the
> problem.

Do you have actually example of bugs this changes introduce (I'm curious)
Adrian Buehlmann - May 5, 2014, 8:37 p.m.
On 2014-05-05 21:37, Pierre-Yves David wrote:
> 
> 
> On 05/05/2014 11:56 AM, Steve Borho wrote:
>> On Sat, May 3, 2014 at 3:43 AM, Adrian Buehlmann <adrian@cadifra.com> wrote:
>>> # HG changeset patch
>>> # User Adrian Buehlmann <adrian@cadifra.com>
>>> # Date 1399106034 -7200
>>> # Branch stable
>>> # Node ID b3bceb2a103e9f279f058e7a3bacf272968b6bb6
>>> # Parent  d36440d843284ba546858b241da9cc4817811fe5
>>> win32: backout 1a9ebc83a74c
>>>
>>> This appears to cause havoc on TortoiseHg. While 1a9ebc83a74c may be nice to
>>> have (as soon as someone can prove it has no unwanted side effects), it is not
>>> worth the troubles at this point.
>>
>> To add some context; the troubles are specifically that files which
>> are being "watched" for changes by QFileSystemWatcher() are no longer
>> safely updated by util.rename().  So things like updating .hg/hgrc or
>> perhaps even the changelog are suddenly dangerous.  We're still not
>> completely certain of the exact semantics of the problem so I don't
>> know if this patch is mandatory, but we believe it would resolve the
>> problem.
> 
> Do you have actually example of bugs this changes introduce (I'm curious)

[It's nice to see you being curious about hairy dirty win32 details. :-) ]

Yep. But only together with TortoiseHg on Windows, which uses this layer
of Mercurial:

Steve lost his hgrc file on a local disk (as he reported), which is
enough evidence for me to backout 1a9ebc83a74c.

1a9ebc83a74c was the only change on that layer, which has always been a
delicate matter (Windows file access trying to emulate posix
expectations of Mercurial, combined with more or less evil virus
scanner-like environments).

I really can no longer recommend to inflict 1a9ebc83a74c on everyone.
TortoiseHg replacing win32.unlink is not really an option either, as
that would reduce usage of this part of Mercurial to an almost
negligible quantity.

It wasn't found out earlier, apparently because no one built a
TortoiseHg with 1a9ebc83a74c and used it. Steve was the guinea pig himself.

1a9ebc83a74c is a performance optimization for a non-essential use case.
Not worth the trouble, IMHO.

In fact I strongly recommend against publishing a TortoiseHg binary
containing 1a9ebc83a74c.

The other option would be to find someone who is cute enough to release
such a binary and wait for another one which either proves all works
fine or presents a theory of why 1a9ebc83a74c is harmful exactly.

I won't be neither one.

And Steve has already publicly declared that he will hold off tagging a
TortoiseHg release until this issue is resolved.

So, either we find hard evidence or we just backout.
Steve Borho - May 5, 2014, 10:45 p.m.
On Mon, May 5, 2014 at 3:37 PM, Adrian Buehlmann <adrian@cadifra.com> wrote:
> On 2014-05-05 21:37, Pierre-Yves David wrote:
>>
>>
>> On 05/05/2014 11:56 AM, Steve Borho wrote:
>>> On Sat, May 3, 2014 at 3:43 AM, Adrian Buehlmann <adrian@cadifra.com> wrote:
>>>> # HG changeset patch
>>>> # User Adrian Buehlmann <adrian@cadifra.com>
>>>> # Date 1399106034 -7200
>>>> # Branch stable
>>>> # Node ID b3bceb2a103e9f279f058e7a3bacf272968b6bb6
>>>> # Parent  d36440d843284ba546858b241da9cc4817811fe5
>>>> win32: backout 1a9ebc83a74c
>>>>
>>>> This appears to cause havoc on TortoiseHg. While 1a9ebc83a74c may be nice to
>>>> have (as soon as someone can prove it has no unwanted side effects), it is not
>>>> worth the troubles at this point.
>>>
>>> To add some context; the troubles are specifically that files which
>>> are being "watched" for changes by QFileSystemWatcher() are no longer
>>> safely updated by util.rename().  So things like updating .hg/hgrc or
>>> perhaps even the changelog are suddenly dangerous.  We're still not
>>> completely certain of the exact semantics of the problem so I don't
>>> know if this patch is mandatory, but we believe it would resolve the
>>> problem.
>>
>> Do you have actually example of bugs this changes introduce (I'm curious)
>
> [It's nice to see you being curious about hairy dirty win32 details. :-) ]
>
> Yep. But only together with TortoiseHg on Windows, which uses this layer
> of Mercurial:
>
> Steve lost his hgrc file on a local disk (as he reported), which is
> enough evidence for me to backout 1a9ebc83a74c.
>
> 1a9ebc83a74c was the only change on that layer, which has always been a
> delicate matter (Windows file access trying to emulate posix
> expectations of Mercurial, combined with more or less evil virus
> scanner-like environments).
>
> I really can no longer recommend to inflict 1a9ebc83a74c on everyone.
> TortoiseHg replacing win32.unlink is not really an option either, as
> that would reduce usage of this part of Mercurial to an almost
> negligible quantity.
>
> It wasn't found out earlier, apparently because no one built a
> TortoiseHg with 1a9ebc83a74c and used it. Steve was the guinea pig himself.
>
> 1a9ebc83a74c is a performance optimization for a non-essential use case.
> Not worth the trouble, IMHO.
>
> In fact I strongly recommend against publishing a TortoiseHg binary
> containing 1a9ebc83a74c.
>
> The other option would be to find someone who is cute enough to release
> such a binary and wait for another one which either proves all works
> fine or presents a theory of why 1a9ebc83a74c is harmful exactly.
>
> I won't be neither one.
>
> And Steve has already publicly declared that he will hold off tagging a
> TortoiseHg release until this issue is resolved.
>
> So, either we find hard evidence or we just backout.

I built two packages today to test this issue.

The first package has Mercurial 3.0 and a slightly old version of
TortoiseHg (prior to some workarounds we made for this issue).  And
with this package I was able to reproduce the problem very quickly by
editing a repository .hg/hgrc file using our settings tool (which uses
util.atomictempfile to swap the old with the new atomically).

The second package was the same as the first except I backed out
1a9ebc83a74c in Mercurial and manually moved the 3.0 tag locally to
include that backout (I have to move tags in order to build
test-release packages).

With the second package I was unable to reproduce the file overwrite errors.

Patch

diff --git a/mercurial/win32.py b/mercurial/win32.py
--- a/mercurial/win32.py
+++ b/mercurial/win32.py
@@ -24,7 +24,6 @@ 
 
 # GetLastError
 _ERROR_SUCCESS = 0
-_ERROR_SHARING_VIOLATION = 32
 _ERROR_INVALID_PARAMETER = 87
 _ERROR_INSUFFICIENT_BUFFER = 122
 
@@ -60,9 +59,7 @@ 
 
 _OPEN_EXISTING = 3
 
-_FILE_FLAG_OPEN_REPARSE_POINT = 0x00200000
 _FILE_FLAG_BACKUP_SEMANTICS = 0x02000000
-_FILE_FLAG_DELETE_ON_CLOSE = 0x04000000
 
 # SetFileAttributes
 _FILE_ATTRIBUTE_NORMAL = 0x80
@@ -423,18 +420,11 @@ 
 def unlink(f):
     '''try to implement POSIX' unlink semantics on Windows'''
 
-    # If we can open f exclusively, no other processes must have open handles
-    # for it and we can expect its name will be deleted immediately when we
-    # close the handle unless we have another in the same process.  We also
-    # expect we shall simply fail to open f if it is a directory.
-    fh = _kernel32.CreateFileA(f, 0, 0, None, _OPEN_EXISTING,
-        _FILE_FLAG_OPEN_REPARSE_POINT | _FILE_FLAG_DELETE_ON_CLOSE, None)
-    if fh != _INVALID_HANDLE_VALUE:
-        _kernel32.CloseHandle(fh)
-        return
-    error = _kernel32.GetLastError()
-    if error != _ERROR_SHARING_VIOLATION:
-        raise ctypes.WinError(error)
+    if os.path.isdir(f):
+        # use EPERM because it is POSIX prescribed value, even though
+        # unlink(2) on directories returns EISDIR on Linux
+        raise IOError(errno.EPERM,
+                      "Unlinking directory not permitted: '%s'" % f)
 
     # POSIX allows to unlink and rename open files. Windows has serious
     # problems with doing that: