bug#22534: File notify broken on Windows

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 2990 bytes --]

When I run 'make check', the process is stuck on filenotify tests.
If I try to run the tests interactively, I have to hit 'Ctrl-g'.
Emacs waits forever on the first test.



In GNU Emacs 25.0.90.4 (x86_64-w64-mingw32)
 of 2016-02-02 built on LOBSANG
Repository revision: 17b39a44e9106bae6c084441a29c5ec77f36a721
Windowing system distributor 'Microsoft Corp.', version 10.0.10586
Configured using:
 'configure --prefix=/c/Local/Emacs --libexecdir=/c/Local/Emacs/bin
 --datarootdir=/c/Local/Emacs --localstatedir=/c/Local/Emacs
 --sysconfdir=/c/Local/Emacs/etc --with-jpeg --with-xpm --with-png
 --with-tiff --with-rsvg --with-xml2 --with-gnutls --without-imagemagick
 --enable-checking=no 'CFLAGS=-I/mingw64/include -fomit-frame-pointer
 -O3 -g0 -mtune=corei7' CPPFLAGS=-I/mingw64/include
 LDFLAGS=-L/mingw64/lib'

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND DBUS NOTIFY ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS

Important settings:
  value of $LANG: fr_FR
  locale-coding-system: cp1252

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Making completion list...

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message dired format-spec rfc822 mml
mml-sec epg epg-config gnus-util mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util help-fns help-mode easymenu cl-loaddefs pcase
cl-lib mail-prsvr mail-utils time-date mule-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel dos-w32 ls-lisp
disp-table w32-win w32-vars term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list newcomment elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core frame cl-generic cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese charscript case-table epa-hook
jka-cmpr-hook help simple abbrev minibuffer cl-preloaded nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote w32notify dbusbind w32 multi-tty
make-network-process emacs)

Memory information:
((conses 16 88664 8174)
 (symbols 56 19615 0)
 (miscs 48 37 84)
 (strings 32 15795 4324)
 (string-bytes 1 424760)
 (vectors 16 11609)
 (vector-slots 8 411396 4797)
 (floats 8 161 40)
 (intervals 56 256 6)
 (buffers 976 12))

[-- Attachment #2: Type: text/html, Size: 4003 bytes --]

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Tue, 2 Feb 2016 07:23:43 +0100
> 
> When I run 'make check', the process is stuck on filenotify tests.
> If I try to run the tests interactively, I have to hit 'Ctrl-g'.
> Emacs waits forever on the first test.

Can you bisect to find the commit which broke this?

Thanks.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 3722 bytes --]

You will find the commit below (with not too much surprise).
However, I don't have much time these days to sort this out.

Fabrice

$ git bisect good
7bf54d01159eb09bae3c9cd86f2af0812d9afdf6 is the first bad commit
commit 7bf54d01159eb09bae3c9cd86f2af0812d9afdf6
Author: Michael Albinus <michael.albinus@gmx.de>
Date:   Fri Jan 22 19:56:09 2016 +0100

    Backport kqueue integration from master

    * configure.ac (--with-file-notification): Add kqueue.
    (top): Remove special test for "${HAVE_NS}" and
    ${with_file_notification}, this is handled inside gfilenotify
    tests.  Add kqueue tests.  Use NOTIFY_CFLAGS and NOTIFY_LIBS
    instead of library specific variables.  Add error message for
    gfile on Nextstep.

    * doc/lispref/os.texi (File Notifications): Add kqueue as backend.
    Fix some glitches in the example.

    * etc/NEWS: Mention kqueue.

    * lisp/filenotify.el (file-notify--library)
    (file-notify-descriptors, file-notify-callback)
    (file-notify-add-watch, file-notify-rm-watch)
    (file-notify-valid-p): Add kqueue support.
    (file-notify--rm-descriptor): Remove WHAT arg.

    * src/Makefile.in: Use NOTIFY_CFLAGS and NOTIFY_LIBS.

    * src/emacs.c (main): Call globals_of_kqueue and syms_of_kqueue.

    * src/inotify.c (inotifyevent_to_event): Extract file name from
    watch_object if the event doesn't provide it.
    (Finotify_add_watch): Add file name to watch_object.

    * src/keyboard.c (make_lispy_event): Check also for HAVE_KQUEUE.

    * src/kqueue.c: New file.

    * src/lisp.h: Declare extern globals_of_kqueue and syms_of_kqueue.

    * test/automated/file-notify-tests.el
    (file-notify--test-expected-events): Remove.
    (file-notify--test-cleanup): Do not set that variable.
    (file-notify--test-timeout) Use different timeouts for
    different libraries.
    (file-notify--test-library): New defun.
    (file-notify--test-event-test): Make stronger checks.
    (file-notify--test-with-events): EVENTS can also be a list of
    lists.  Flush outstanding events before running the body.
    Make timeout heuristically depend on the number of events.
    (file-notify-test01-add-watch, file-notify-test02-events)
    (file-notify-test04-file-validity, file-notify-test05-dir-validity):
    Rewrite in order to call file monitors but directory monitors.
    (file-notify-test02-events, file-notify-test04-file-validity): Do
    not skip cygwin tests.  Add additional test for file creation.
    Adapt expected result for different backends.
    (file-notify-test03-autorevert): Some of the tests don't work for
    w32notify.
    (file-notify-test06-many-events): New test.

:100644 100644 b5e6b77c713ca7fa82c7c23652170dff3c5adfd5
76193fae6dd61d4448b3a304d3a0e4978afb810e M      configure.ac
:040000 040000 6b5e2cfa75a456e6f4d43e183976d6e8ece21462
41f9bd9c7893cbdccf22e97d7ddc3500c4443d5c M      doc
:040000 040000 98523d83bfa75e0401f7a1049548c4f94ddb6b0d
4a87e49af135328911e0b31099f0e9d0f4f13aaf M      etc
:040000 040000 96c8883a122a08fe59a772611eff59a8330a37c1
8e04830234a1d3e8af4fd37ceb3eb9c4503718c3 M      lisp
:040000 040000 ebdfee938f44d5d2818a29bd07f795521a3583ee
53a01fd613d99e20cb563ab8418ec2776c146f09 M      src
:040000 040000 9d27cfce0b9d77d43b2180ecd95451469309da92
71329cdc731a284e563265a90876cbf8ea94305b M      test


2016-02-02 17:15 GMT+01:00 Eli Zaretskii <eliz@gnu.org>:

> > From: Fabrice Popineau <fabrice.popineau@gmail.com>
> > Date: Tue, 2 Feb 2016 07:23:43 +0100
> >
> > When I run 'make check', the process is stuck on filenotify tests.
> > If I try to run the tests interactively, I have to hit 'Ctrl-g'.
> > Emacs waits forever on the first test.
>
> Can you bisect to find the commit which broke this?
>
> Thanks.
>

[-- Attachment #2: Type: text/html, Size: 5164 bytes --]

bug#22534: File notify broken on Windows

[Michael Albinus]

Fabrice Popineau <fabrice.popineau@gmail.com> writes:

> You will find the commit below (with not too much surprise).
> However, I don't have much time these days to sort this out.

I'll check next days, need to find an MS Windows machine first.

> Fabrice

Best regards, Michael.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

2016-02-03 9:07 GMT+01:00 Michael Albinus <michael.albinus@gmx.de>:

> Fabrice Popineau <fabrice.popineau@gmail.com> writes:
>
> > You will find the commit below (with not too much surprise).
> > However, I don't have much time these days to sort this out.
>
> I'll check next days, need to find an MS Windows machine first.
>
>
Thanks Michael.

I have observed something weird during the file-notify tests.
I traced file-notify-test02-events.
The read_event function doesn't time out in 0.1s as it is supposed to.
It waits for the time specified  by (file-notify--test-timeout) before
returning the right value. (Well actually, the value returned by the third
test
in file-notify-test02-events is wrong but it is another story).

So I'm afraid some logic is wrong, and possibly not only in file-notify
but also in the way w32 should handle those timeouts.

Fabrice

[-- Attachment #2: Type: text/html, Size: 1517 bytes --]

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

A couple of my findings :

- notifications returned are not the same whether you run the tests in
batch mode or interactive mode.
  In interactive mode, there is a deleted notification which is sent when
your remove the directory being watched.
  This event is not seen when running in batch mode (make check). I wonder
what could make a difference.
- in the test file-notify-test06-many-events to check that events are not
dropped : I have to lower the 1000 number.
  The test fails as soon as I go higher than around 260. Is there some
limit here ? Once the limit is reached, only
  the first notification is returned.
- there is this small patch that needs to be applied :

diff --git a/test/automated/file-notify-tests.el
b/test/automated/file-notify-tests.el
index de64f50..51a898e 100644
--- a/test/automated/file-notify-tests.el
+++ b/test/automated/file-notify-tests.el
@@ -507,7 +509,7 @@ file-notify--test-with-events
               ;; w32notify does not distinguish between `changed' and
               ;; `attribute-changed'.
               ((string-equal (file-notify--test-library) "w32notify")
-               '(changed changed changed changed))
+               '(changed changed))
               ;; For kqueue and in the remote case, `write-region'
               ;; raises also an `attribute-changed' event.
               ((or (string-equal (file-notify--test-library) "kqueue")

Overall, I don't think anymore that the patch by Michael has broken w32
file notifications
but rather that the new tests have highlighted some potential problems with
it.

Regards,

Fabrice


2016-02-03 13:15 GMT+01:00 Fabrice Popineau <fabrice.popineau@gmail.com>:

>
>
> 2016-02-03 9:07 GMT+01:00 Michael Albinus <michael.albinus@gmx.de>:
>
>> Fabrice Popineau <fabrice.popineau@gmail.com> writes:
>>
>> > You will find the commit below (with not too much surprise).
>> > However, I don't have much time these days to sort this out.
>>
>> I'll check next days, need to find an MS Windows machine first.
>>
>>
> Thanks Michael.
>
> I have observed something weird during the file-notify tests.
> I traced file-notify-test02-events.
> The read_event function doesn't time out in 0.1s as it is supposed to.
> It waits for the time specified  by (file-notify--test-timeout) before
> returning the right value. (Well actually, the value returned by the third
> test
> in file-notify-test02-events is wrong but it is another story).
>
> So I'm afraid some logic is wrong, and possibly not only in file-notify
> but also in the way w32 should handle those timeouts.
>
> Fabrice
>
>

[-- Attachment #2: Type: text/html, Size: 4024 bytes --]

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Thu, 4 Feb 2016 10:49:42 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, 22534@debbugs.gnu.org
> 
> - notifications returned are not the same whether you run the tests in batch mode or interactive mode.
> In interactive mode, there is a deleted notification which is sent when your remove the directory being
> watched.
> This event is not seen when running in batch mode (make check). I wonder what could make a difference.

Support for file notifications in batch mode is fragile: w32notify
normally works by sending a message to the main thread whenever it has
a notification to report.  But in batch mode, the main thread doesn't
read Windows messages, so whether an event gets reported depends on
whether 'pselect' is called, which depends on what API is called to
wait for notifications and read them.

> - in the test file-notify-test06-many-events to check that events
> are not dropped : I have to lower the 1000 number.  The test fails
> as soon as I go higher than around 260. Is there some limit here ?

Is this in interactive or a batch-mode run?

> Once the limit is reached, only  the first notification is returned.

What do you mean by "the first notification"?  AFAIU, the 1000 events
are generated as 500 pairs of renames, so what is "the first" here,
after 260 were already generated?

> Overall, I don't think anymore that the patch by Michael has broken w32 file notifications
> but rather that the new tests have highlighted some potential problems with it.

I've just found a serious problem, see bug#22557.  I'm not sure it is
related to what you see here, but IMO until that bug is resolved,
there's no sense in trying to analyze what happens with notifications
on MS-Windows.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 2382 bytes --]

2016-02-04 19:47 GMT+01:00 Eli Zaretskii <eliz@gnu.org>:

> > From: Fabrice Popineau <fabrice.popineau@gmail.com>
> > Date: Thu, 4 Feb 2016 10:49:42 +0100
> > Cc: Eli Zaretskii <eliz@gnu.org>, 22534@debbugs.gnu.org
> >
> > - notifications returned are not the same whether you run the tests in
> batch mode or interactive mode.
> > In interactive mode, there is a deleted notification which is sent when
> your remove the directory being
> > watched.
> > This event is not seen when running in batch mode (make check). I wonder
> what could make a difference.
>
> Support for file notifications in batch mode is fragile: w32notify
> normally works by sending a message to the main thread whenever it has
> a notification to report.  But in batch mode, the main thread doesn't
> read Windows messages, so whether an event gets reported depends on
> whether 'pselect' is called, which depends on what API is called to
> wait for notifications and read them.
>
>
Ok. Maybe that's the reason, but in this case, the expected events have to
be adjusted because 'make check'
runs in batch mode.


> > - in the test file-notify-test06-many-events to check that events
> > are not dropped : I have to lower the 1000 number.  The test fails
> > as soon as I go higher than around 260. Is there some limit here ?
>
> Is this in interactive or a batch-mode run?
>
> In interactive mode. The limit is somewhere between 260 and 280.


> > Once the limit is reached, only  the first notification is returned.
>
> What do you mean by "the first notification"?  AFAIU, the 1000 events
> are generated as 500 pairs of renames, so what is "the first" here,
> after 260 were already generated?
>
>
The notification for the first pair in the list, the one numbered 999 is
the only one
appearing in file-notify--test-events.


> > Overall, I don't think anymore that the patch by Michael has broken w32
> file notifications
> > but rather that the new tests have highlighted some potential problems
> with it.
>
> I've just found a serious problem, see bug#22557.  I'm not sure it is
> related to what you see here, but IMO until that bug is resolved,
> there's no sense in trying to analyze what happens with notifications
> on MS-Windows.
>

Ok, I have seen that. Maybe I'll try to comment out the part of the patch
you have pointed out in this bug report
and see how it changes the behavior.

Fabrice

[-- Attachment #2: Type: text/html, Size: 3996 bytes --]

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Fri, 5 Feb 2016 06:59:00 +0100
> Cc: Michael Albinus <michael.albinus@gmx.de>, 22534@debbugs.gnu.org
> 
>  Support for file notifications in batch mode is fragile: w32notify
>  normally works by sending a message to the main thread whenever it has
>  a notification to report. But in batch mode, the main thread doesn't
>  read Windows messages, so whether an event gets reported depends on
>  whether 'pselect' is called, which depends on what API is called to
>  wait for notifications and read them.
> 
> Ok. Maybe that's the reason, but in this case, the expected events have to be adjusted because 'make check'
> runs in batch mode.

True.  I have done that several times to fix that test; maybe it's
time to do that again.

>  > Once the limit is reached, only the first notification is returned.
> 
>  What do you mean by "the first notification"? AFAIU, the 1000 events
>  are generated as 500 pairs of renames, so what is "the first" here,
>  after 260 were already generated?
> 
> The notification for the first pair in the list, the one numbered 999 is the only one
> appearing in file-notify--test-events.

So you are saying that as soon as the value of n is greater than some
threshold, like 260, the test reports only the first pair of renames,
is that true?

If so, did you try to insert waits in-between the series or renames?
Or maybe the preceding series of file creations is the culprit, and we
should give Emacs more time to report them?
Because right now, it just fires up all of them in a single quick
series.

Hmm... actually, I might see a problem.  If my reading of the test is
correct, it first creates 2000 files, and then issues 1000 renames,
which on Windows generate 2000 notifications.  Together, these add up
to 4000, which is awfully close to the 4096 size of the Emacs input
event queue.  So maybe the test fills up the event queue, and the
synchronization between the w32notify thread and the main thread stops
working at that point?  Or maybe we are hitting on some limit of
Windows messages that can be enqueued?

(To tell the truth, I'm not sure what is the purpose of that test:
AFAIK, none of the available notification back-ends ever promised not
to lose events, so what are we testing here?  Perhaps Michael can
explain.)

>  I've just found a serious problem, see bug#22557. I'm not sure it is
>  related to what you see here, but IMO until that bug is resolved,
>  there's no sense in trying to analyze what happens with notifications
>  on MS-Windows.
> 
> Ok, I have seen that. Maybe I'll try to comment out the part of the patch you have pointed out in this bug report
> and see how it changes the behavior.

If you have time, yes, please.

Btw, are you doing this on master or on emacs-25?  The files involved
seem to be identical for now, but they might diverge in the future.

Thanks.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 3172 bytes --]

2016-02-05 11:10 GMT+01:00 Eli Zaretskii <eliz@gnu.org>:

> > From: Fabrice Popineau <fabrice.popineau@gmail.com>
> > Date: Fri, 5 Feb 2016 06:59:00 +0100
> > Cc: Michael Albinus <michael.albinus@gmx.de>, 22534@debbugs.gnu.org
> >
>
> >  > Once the limit is reached, only the first notification is returned.
> >
> >  What do you mean by "the first notification"? AFAIU, the 1000 events
> >  are generated as 500 pairs of renames, so what is "the first" here,
> >  after 260 were already generated?
> >
> > The notification for the first pair in the list, the one numbered 999 is
> the only one
> > appearing in file-notify--test-events.
>
> So you are saying that as soon as the value of n is greater than some
> threshold, like 260, the test reports only the first pair of renames,
> is that true?
>
>
Yes, this is what I observe with different values of n.



> If so, did you try to insert waits in-between the series or renames?
>

I have tried to add a sit-for in the loop which renames the files, but
it does not help.


> Or maybe the preceding series of file creations is the culprit, and we
> should give Emacs more time to report them?
>

I have tried with different values of file-notify--test-timeout but it
doesn't seem
to make any difference. I also boosted the size of the file_notifications[]
array (* 16)
without any difference. I have tested with sit-for between each rename
operation
and it does not work either above 260 files



Because right now, it just fires up all of them in a single quick
> series.
>
> Hmm... actually, I might see a problem.  If my reading of the test is
> correct, it first creates 2000 files, and then issues 1000 renames,
> which on Windows generate 2000 notifications.  Together, these add up
> to 4000, which is awfully close to the 4096 size of the Emacs input
> event queue.  So maybe the test fills up the event queue, and the
> synchronization between the w32notify thread and the main thread stops
> working at that point?  Or maybe we are hitting on some limit of
> Windows messages that can be enqueued?
>
>
I would have expected some Win32 function to error at some point ?

Also, would it be useful to use the overlap mechanism ? Would it help to
overcome this kind of limitation ?


> (To tell the truth, I'm not sure what is the purpose of that test:
> AFAIK, none of the available notification back-ends ever promised not
> to lose events, so what are we testing here?  Perhaps Michael can
> explain.)
>
> >  I've just found a serious problem, see bug#22557. I'm not sure it is
> >  related to what you see here, but IMO until that bug is resolved,
> >  there's no sense in trying to analyze what happens with notifications
> >  on MS-Windows.
> >
> > Ok, I have seen that. Maybe I'll try to comment out the part of the
> patch you have pointed out in this bug report
> > and see how it changes the behavior.
>
> If you have time, yes, please.
>
>
It does not help with file-notify-test06-many-events but I would have
expected it.


> Btw, are you doing this on master or on emacs-25?  The files involved
> seem to be identical for now, but they might diverge in the future.
>

emacs-25 at this time.

Fabrice

[-- Attachment #2: Type: text/html, Size: 5245 bytes --]

bug#22534: File notify broken on Windows

[Michael Albinus]

Fabrice Popineau <fabrice.popineau@gmail.com> writes:

Hi,

> I have tried with different values of file-notify--test-timeout but it
> doesn't seem to make any difference. I also boosted the size of the
> file_notifications[] array (* 16) without any difference. I have
> tested with sit-for between each rename operation and it does not work
> either above 260 files

Is it worth to continue to look for the problem in w32notify? I would simply
set the loop limit there to 250, that's it.

file-notify-test06-many-events is a stress test. Originally, Wolfgang
Jenkner proposed it with 10.000 iterations - that was *really* much too
long.

[Eli]

>     (To tell the truth, I'm not sure what is the purpose of that test:
>     AFAIK, none of the available notification back-ends ever promised
>     not to lose events, so what are we testing here? Perhaps Michael
>     can explain.)

As said, it was intended as stress test. But we are free to adapt it to
the limitations of the underlying backend.

> Fabrice

Best regards, Michael.

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  22534@debbugs.gnu.org
> Date: Fri, 05 Feb 2016 18:18:59 +0100
> 
> > I have tried with different values of file-notify--test-timeout but it
> > doesn't seem to make any difference. I also boosted the size of the
> > file_notifications[] array (* 16) without any difference. I have
> > tested with sit-for between each rename operation and it does not work
> > either above 260 files
> 
> Is it worth to continue to look for the problem in w32notify? I would simply
> set the loop limit there to 250, that's it.

It would be a waste of your time to look into that.  Go ahead and
lower the number; I will look into the problem when I have time and
see what come up with.  We can then pick up where we left off.

> file-notify-test06-many-events is a stress test. Originally, Wolfgang
> Jenkner proposed it with 10.000 iterations - that was *really* much too
> long.
> 
> [Eli]
> 
> >     (To tell the truth, I'm not sure what is the purpose of that test:
> >     AFAIK, none of the available notification back-ends ever promised
> >     not to lose events, so what are we testing here? Perhaps Michael
> >     can explain.)
> 
> As said, it was intended as stress test. But we are free to adapt it to
> the limitations of the underlying backend.

My point is that we could legitimately lose events for reasons that
have nothing to do with Emacs.

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Is it worth to continue to look for the problem in w32notify? I would simply
>> set the loop limit there to 250, that's it.
>
> It would be a waste of your time to look into that.  Go ahead and
> lower the number; I will look into the problem when I have time and
> see what come up with.  We can then pick up where we left off.

Done.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> Date: Fri, 05 Feb 2016 21:43:26 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 22534@debbugs.gnu.org, fabrice.popineau@gmail.com
> 
> > From: Michael Albinus <michael.albinus@gmx.de>
> > Cc: Eli Zaretskii <eliz@gnu.org>,  22534@debbugs.gnu.org
> > Date: Fri, 05 Feb 2016 18:18:59 +0100
> > 
> > > I have tried with different values of file-notify--test-timeout but it
> > > doesn't seem to make any difference. I also boosted the size of the
> > > file_notifications[] array (* 16) without any difference. I have
> > > tested with sit-for between each rename operation and it does not work
> > > either above 260 files
> > 
> > Is it worth to continue to look for the problem in w32notify? I would simply
> > set the loop limit there to 250, that's it.
> 
> It would be a waste of your time to look into that.  Go ahead and
> lower the number; I will look into the problem when I have time and
> see what come up with.  We can then pick up where we left off.

I've looked into this.  The simple patch below fixes the test for me.

The problem was that all the 1000 renames were run in a single long
series.  On MS-Windows this generates 2000 events, because the target
file exists and needs to be deleted first.  However, w32notify has a
static limit of about 1000 events it can collect without losing some
(that's on a 32-bit system, probably less on a 64-bit OS).  The last
part of the riddle is that in batch mode file notifications are not
read on MS-Windows unless someone calls read-event or a similar
function that calls 'pselect' to check for input.  If they are not
read, they keep accumulating, and are then reported all together.

I also suspect that the Windows API use by w32notify has its own limit
of events it can accumulate, and beyond that limit simply drops the
older events on the floor.

The solution is to call read-event while renaming the files.  Then I
could go back to 1000 renames without failures.

Michael, is there a reason not to issue those additional calls to
read-event on other platforms?  If necessary, those calls could be
made conditional on MS-Windows.

Here's the proposed patch.  Fabrice, please see if this works for you
(the full test runs for about 7 minutes, so be patient).  Btw, we
could make the test run faster by lowering the 0.1 argument to the 2
read-event calls while we create the 2000 files at the beginning of
the test, I see no need for waiting that long for a single file
creation.

diff --git a/test/automated/file-notify-tests.el b/test/automated/file-notify-tests.el
index 629d85b..5fc4ff8 100644
--- a/test/automated/file-notify-tests.el
+++ b/test/automated/file-notify-tests.el
@@ -66,7 +66,7 @@ file-notify--test-timeout
   "Timeout to wait for arriving events, in seconds."
   (cond
    ((file-remote-p temporary-file-directory) 6)
-   ((string-equal (file-notify--test-library) "w32notify") 20)
+   ((string-equal (file-notify--test-library) "w32notify") 10)
    ((eq system-type 'cygwin) 10)
    (t 3)))
 
@@ -797,10 +797,7 @@ file-notify--test-with-events
 	  file-notify--test-tmpfile
 	  '(change) 'file-notify--test-event-handler)))
   (unwind-protect
-      ;; In case of w32notify, the upper limit of events to handle
-      ;; seems to be 260.  Reason unknown.
-      (let ((n (if (string-equal (file-notify--test-library) "w32notify")
-                   250 1000))
+      (let ((n 1000)
             source-file-list target-file-list
             (default-directory file-notify--test-tmpfile))
         (dotimes (i n)
@@ -832,10 +829,11 @@ file-notify--test-with-events
           (let ((source-file-list source-file-list)
                 (target-file-list target-file-list))
             (while (and source-file-list target-file-list)
-              (rename-file (pop source-file-list) (pop target-file-list) t))))
+              (rename-file (pop source-file-list) (pop target-file-list) t)
+              (read-event nil nil 0.02))))
         (file-notify--test-with-events (make-list n 'deleted)
           (dolist (file target-file-list)
-            (delete-file file))))
+            (prog1 (delete-file file) (read-event nil nil 0.02)))))
     (file-notify--test-cleanup)))
 
 (file-notify--deftest-remote file-notify-test06-many-events

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> The solution is to call read-event while renaming the files.  Then I
> could go back to 1000 renames without failures.
>
> Michael, is there a reason not to issue those additional calls to
> read-event on other platforms?  If necessary, those calls could be
> made conditional on MS-Windows.

No, it's OK also for the other backends. This might make the test even
more robust.

> Here's the proposed patch.  Fabrice, please see if this works for you
> (the full test runs for about 7 minutes, so be patient).  Btw, we
> could make the test run faster by lowering the 0.1 argument to the 2
> read-event calls while we create the 2000 files at the beginning of
> the test, I see no need for waiting that long for a single file
> creation.

Well, the two Tramp backends aren't such fast. That's why I use a 0.1
sec timeout for all read-event calls. Maybe we could introduce a
defconst for this, setting it to 0.02 for local backends, and to 0.1 for
remote backends.

Please install the patch; I'll test it then for the other 5 backends
next days.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 22534@debbugs.gnu.org,  fabrice.popineau@gmail.com
> Date: Sat, 06 Feb 2016 20:24:32 +0100
> 
> Please install the patch

Done.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 3278 bytes --]

The situation has improved especially with file-notify-test06-many-events,
but I still see problems in batch mode.
The remaining problems are in file-notify-test02-events.

In the 'check for attribute change', I see only 2 'changed' notifications,
and not 4
as expected by the test (around line 512: w32notify does not distinguish
between
'changed' and 'attribute-changed').

So I need to apply :

diff --git a/test/automated/file-notify-tests.el
b/test/automated/file-notify-tests.el
index 5fc4ff8..943bd7e 100644
--- a/test/automated/file-notify-tests.el
+++ b/test/automated/file-notify-tests.el
@@ -507,7 +512,7 @@ file-notify--test-with-events
                ;; w32notify does not distinguish between `changed' and
                ;; `attribute-changed'.
                ((string-equal (file-notify--test-library) "w32notify")
-               '(changed changed changed changed))
+               '(changed changed))
                ;; For kqueue and in the remote case, `write-region'
                ;; raises also an `attribute-changed' event.
                ((or (string-equal (file-notify--test-library) "kqueue")

For the rest of this test, whereas the test passes in interactive mode, it
fails in batch mode with
several 'deleted' notifications. Namely, I need to apply the following for
the test to pass.

diff --git a/test/automated/file-notify-tests.el
b/test/automated/file-notify-tests.el
index 5fc4ff8..943bd7e 100644
--- a/test/automated/file-notify-tests.el
+++ b/test/automated/file-notify-tests.el
@@ -396,7 +397,8 @@ file-notify--test-with-events
                ;; w32notify does raise a `stopped' event when a
                ;; watched directory is deleted.
                ((string-equal (file-notify--test-library) "w32notify")
-               '(created changed deleted))
+               '(created changed ; deleted
+                          ))
                ;; cygwin recognizes only `deleted' and `stopped' events.
                ((eq system-type 'cygwin)
                '(deleted stopped))
@@ -430,7 +432,8 @@ file-notify--test-with-events
                ;; `attribute-changed'.
                ((string-equal (file-notify--test-library) "w32notify")
                '(created changed created changed changed changed changed
-                 deleted deleted))
+                          ;; deleted deleted
+                          ))
                ;; cygwin recognizes only `deleted' and `stopped' events.
                ((eq system-type 'cygwin)
                '(deleted stopped))
@@ -479,6 +482,8 @@ file-notify--test-with-events
                ;; the directory.  Except for kqueue.
                ((string-equal (file-notify--test-library) "kqueue")
                '(created changed renamed deleted stopped))
+               ((string-equal (file-notify--test-library) "w32notify")
+               '(created changed renamed))
                (t '(created changed renamed deleted deleted stopped)))
             (read-event nil nil 0.1)
             (write-region


Fabrice

2016-02-06 20:55 GMT+01:00 Eli Zaretskii <eliz@gnu.org>:

> > From: Michael Albinus <michael.albinus@gmx.de>
> > Cc: 22534@debbugs.gnu.org,  fabrice.popineau@gmail.com
> > Date: Sat, 06 Feb 2016 20:24:32 +0100
> >
> > Please install the patch
>
> Done.
>

[-- Attachment #2: Type: text/html, Size: 4872 bytes --]

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Sun, 7 Feb 2016 14:37:34 +0100
> Cc: Michael Albinus <michael.albinus@gmx.de>, 22534@debbugs.gnu.org
> 
> In the 'check for attribute change', I see only 2 'changed' notifications, and not 4
> as expected by the test (around line 512: w32notify does not distinguish between 
> 'changed' and 'attribute-changed').
> 
> So I need to apply :
> 
> diff --git a/test/automated/file-notify-tests.el b/test/automated/file-notify-tests.el
> index 5fc4ff8..943bd7e 100644
> --- a/test/automated/file-notify-tests.el
> +++ b/test/automated/file-notify-tests.el
> @@ -507,7 +512,7 @@ file-notify--test-with-events
> ;; w32notify does not distinguish between `changed' and
> ;; `attribute-changed'.
> ((string-equal (file-notify--test-library) "w32notify")
> - '(changed changed changed changed))
> + '(changed changed))
> ;; For kqueue and in the remote case, `write-region'
> ;; raises also an `attribute-changed' event.
> ((or (string-equal (file-notify--test-library) "kqueue")

Can you instrument filenotify.el to show the w32notify events this
test generates, complete with the corresponding file names?  I will
then compare to what I see here.

> For the rest of this test, whereas the test passes in interactive mode, it fails in batch mode with
> several 'deleted' notifications. Namely, I need to apply the following for the test to pass.

Does it help to add a call to

  (read-event nil nil 0.1)

after the code that invokes the deletion in each of these cases?

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Can you instrument filenotify.el to show the w32notify events this
> test generates, complete with the corresponding file names?  I will
> then compare to what I see here.

In file-notify-handle-event, there is a debug message. Uncomment it.

>> For the rest of this test, whereas the test passes in interactive
>> mode, it fails in batch mode with
>> several 'deleted' notifications. Namely, I need to apply the
>> following for the test to pass.
>
> Does it help to add a call to
>
>   (read-event nil nil 0.1)
>
> after the code that invokes the deletion in each of these cases?

Hopefully, I'll have access to an MS Windows machine next week. So I
could test in parallel.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 4650 bytes --]

2016-02-07 18:46 GMT+01:00 Eli Zaretskii <eliz@gnu.org>:

> > From: Fabrice Popineau <fabrice.popineau@gmail.com>
> > Date: Sun, 7 Feb 2016 14:37:34 +0100
> > Cc: Michael Albinus <michael.albinus@gmx.de>, 22534@debbugs.gnu.org
> >
> > In the 'check for attribute change', I see only 2 'changed'
> notifications, and not 4
> > as expected by the test (around line 512: w32notify does not distinguish
> between
> > 'changed' and 'attribute-changed').
> >
> > So I need to apply :
> >
> > diff --git a/test/automated/file-notify-tests.el
> b/test/automated/file-notify-tests.el
> > index 5fc4ff8..943bd7e 100644
> > --- a/test/automated/file-notify-tests.el
> > +++ b/test/automated/file-notify-tests.el
> > @@ -507,7 +512,7 @@ file-notify--test-with-events
> > ;; w32notify does not distinguish between `changed' and
> > ;; `attribute-changed'.
> > ((string-equal (file-notify--test-library) "w32notify")
> > - '(changed changed changed changed))
> > + '(changed changed))
> > ;; For kqueue and in the remote case, `write-region'
> > ;; raises also an `attribute-changed' event.
> > ((or (string-equal (file-notify--test-library) "kqueue")
>
> Can you instrument filenotify.el to show the w32notify events this
> test generates, complete with the corresponding file names?  I will
> then compare to what I see here.
>
>
I printed the expected notifications and the ones that occur:

Library: `w32notify'
   passed  1/6  file-notify-test00-availability
   passed  2/6  file-notify-test01-add-watch
events: ((created changed deleted stopped)) ((17923488 created
c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748mQW) (17923488
changed c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748mQW)
(17923488 deleted
c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748mQW) (17923488
stopped c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748mQW))
events: ((changed changed deleted stopped)) ((17490424 changed
c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748zac) (17490424
changed c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748zac)
(17490424 deleted
c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748zac) (17490424
stopped c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748zac))
events: ((created changed deleted)) ((17491048 created
c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo)
(17491048 changed
c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo))
Test file-notify-test02-events backtrace:
  #[0 "\306\307\310C\307C\3111(\312\313\314\315$\317\"\32
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test file-notify-test02-events "Check fi
  ert-run-or-rerun-test([cl-struct-ert--stats (not (tag :expensive-tes
  ert-run-tests((not (tag :expensive-test)) #[385 "\306\307\"\203G\2
  ert-run-tests-batch((not (tag :expensive-test)))
  ert-run-tests-batch-and-exit((not (tag :expensive-test)))
  eval((ert-run-tests-batch-and-exit (quote (not (tag :expensive-test)
  command-line-1(("-L" ";../../../emacs/test/automated" "-l" "ert" "-l
  command-line()
  normal-top-level()
Test file-notify-test02-events condition:
    (ert-test-failed
     ((should
       (dolist
           (elt events result)
         (setq result ...)))
      :form
      (let
          ((--dolist-tail-- events))
        (while --dolist-tail--
          (let ... ... ...))
        result)
      :value nil))
   FAILED  3/6  file-notify-test02-events
Reverting buffer `file-notify-test20748a5u'.
   passed  4/6  file-notify-test03-autorevert
events: ((changed changed deleted stopped)) ((19211556 changed
c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748ZNE) (19211556
changed c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748ZNE)
(19211556 deleted
c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748ZNE) (19211556
stopped c:/Users/Fabrice/AppData/Local/Temp/file-notify-test20748ZNE))
   passed  5/6  file-notify-test04-file-validity
   passed  6/6  file-notify-test05-dir-validity


> > For the rest of this test, whereas the test passes in interactive mode,
> it fails in batch mode with
> > several 'deleted' notifications. Namely, I need to apply the following
> for the test to pass.
>
> Does it help to add a call to
>
>   (read-event nil nil 0.1)
>
> after the code that invokes the deletion in each of these cases?
>

I have added this wait time after each delete-directory call in
file-notify-test02-events
with no difference.

I must add that the same test works interactively.
It is only in batch mode that I don't see the deleted notifications.


Fabrice

[-- Attachment #2: Type: text/html, Size: 6582 bytes --]

bug#22534: File notify broken on Windows

[Michael Albinus]

Fabrice Popineau <fabrice.popineau@gmail.com> writes:

> events: ((created changed deleted)) ((17491048 created
> c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo)
> (17491048 changed
> c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo))

Fails also for me in batch mode. Surprisingly, it doesn't fail always,
sometimes (about 1 in 10 runs) it succeeds here, and fails in the next
step. Strange.

If we do not care the missing `deleted' event in batch mode, I could
make it optional in the test. But maybe it is an indication for a
serious problem in w32notify.

Eli?

> I must add that the same test works interactively. 
> It is only in batch mode that I don't see the deleted notifications.

Same here.

> Fabrice

Best regards, Michael.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

2016-02-08 11:27 GMT+01:00 Michael Albinus <michael.albinus@gmx.de>:

> Fabrice Popineau <fabrice.popineau@gmail.com> writes:
>
> > events: ((created changed deleted)) ((17491048 created
> >
> c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo)
> > (17491048 changed
> >
> c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo))
>
> Fails also for me in batch mode. Surprisingly, it doesn't fail always,
> sometimes (about 1 in 10 runs) it succeeds here, and fails in the next
> step. Strange.
>
> I see the same. The deleted notifications number vary. When you expect 2,
sometimes I see 1.
When you expect one, sometimes I see it. But not frequently.


> If we do not care the missing `deleted' event in batch mode, I could
> make it optional in the test. But maybe it is an indication for a
> serious problem in w32notify.
>
>
Fabrice

[-- Attachment #2: Type: text/html, Size: 1527 bytes --]

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Mon, 8 Feb 2016 11:43:10 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, 22534@debbugs.gnu.org
> 
>  > events: ((created changed deleted)) ((17491048 created
>  > c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo)
>  > (17491048 changed
>  > c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo))
> 
>  Fails also for me in batch mode. Surprisingly, it doesn't fail always,
>  sometimes (about 1 in 10 runs) it succeeds here, and fails in the next
>  step. Strange.
> 
> I see the same. The deleted notifications number vary. When you expect 2, sometimes I see 1.
> When you expect one, sometimes I see it. But not frequently.

Did you try adding calls to read-event after the functions that are
supposed to produce these events?

(I didn't yet have time to look into the traces.  Will do when I have
time.)

Btw, is there an easy way of running just one test out of
file-notify-test.el?  Running all of them takes a long time,
especially now that the 06 test is no longer considered "expensive".

Thanks.

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Btw, is there an easy way of running just one test out of
> file-notify-test.el?  Running all of them takes a long time,
> especially now that the 06 test is no longer considered "expensive".

06 is still expensive, how do you believe it has changed?

"make check" knows the SELECTOR variable. You could do

# make SELECTOR='(quote (eql file-notify-test02-events))' \
  -C test/automated file-notify-tests

> Thanks.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Fabrice Popineau <fabrice.popineau@gmail.com>,  22534@debbugs.gnu.org
> Date: Mon, 08 Feb 2016 20:14:22 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Btw, is there an easy way of running just one test out of
> > file-notify-test.el?  Running all of them takes a long time,
> > especially now that the 06 test is no longer considered "expensive".
> 
> 06 is still expensive, how do you believe it has changed?

Because I see this in file-notify-test.log:

   passed  13/16  file-notify-test06-many-events

Only the remote part of 06 is skipped.  Am I missing something?

> "make check" knows the SELECTOR variable. You could do
> 
> # make SELECTOR='(quote (eql file-notify-test02-events))' \
>   -C test/automated file-notify-tests

OK, thanks.  I wish this were described on some README, or maybe it
could even be less cumbersome to type.

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Because I see this in file-notify-test.log:
>
>    passed  13/16  file-notify-test06-many-events
>
> Only the remote part of 06 is skipped.  Am I missing something?

The remote part is skipped because you are on MS Windows. Expensive
checks are skipped only for "make check" and "make check-maybe". "make
file-notify-tests" run all possible tests in that file.

>> "make check" knows the SELECTOR variable. You could do
>> 
>> # make SELECTOR='(quote (eql file-notify-test02-events))' \
>>   -C test/automated file-notify-tests
>
> OK, thanks.  I wish this were described on some README, or maybe it
> could even be less cumbersome to type.

Description of how to run the tests needs more care. I put it on my
TODO.

The syntax of selectors is given by ert. See (info "(ert) Test Selectors")

Best regards, Michael.

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: fabrice.popineau@gmail.com,  22534@debbugs.gnu.org
> Date: Mon, 08 Feb 2016 21:00:42 +0100
> 
> Expensive checks are skipped only for "make check" and "make
> check-maybe". "make file-notify-tests" run all possible tests in
> that file.

That's inconsistent, won't you agree?

> Description of how to run the tests needs more care. I put it on my
> TODO.

Thanks.

> The syntax of selectors is given by ert. See (info "(ert) Test Selectors")

I understood the syntax by looking at what Makefile does.  But it's
hardly convenient (or pretty) to type Lisp expressions on the Make
command line, IMO.

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Expensive checks are skipped only for "make check" and "make
>> check-maybe". "make file-notify-tests" run all possible tests in
>> that file.
>
> That's inconsistent, won't you agree?

No. If you want to run the tests of a given file, you don't want to
discriminate them. Otherwise, you could run expensive tests in that file
only when you call "make check-expensive". Not acceptable, IMO.

Discriminating the tests is appropriate when running "make check". This
shall give you an overview of Emacs' health in a short time. 

>> The syntax of selectors is given by ert. See (info "(ert) Test Selectors")
>
> I understood the syntax by looking at what Makefile does.  But it's
> hardly convenient (or pretty) to type Lisp expressions on the Make
> command line, IMO.

What would you recommend else? Introduce another syntax for selectors?

Best regards, Michael.

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: fabrice.popineau@gmail.com,  22534@debbugs.gnu.org
> Date: Mon, 08 Feb 2016 21:51:09 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Expensive checks are skipped only for "make check" and "make
> >> check-maybe". "make file-notify-tests" run all possible tests in
> >> that file.
> >
> > That's inconsistent, won't you agree?
> 
> No. If you want to run the tests of a given file, you don't want to
> discriminate them. Otherwise, you could run expensive tests in that file
> only when you call "make check-expensive". Not acceptable, IMO.

That's your logic, not mine.  If I want all the tests, I will say so.

> >> The syntax of selectors is given by ert. See (info "(ert) Test Selectors")
> >
> > I understood the syntax by looking at what Makefile does.  But it's
> > hardly convenient (or pretty) to type Lisp expressions on the Make
> > command line, IMO.
> 
> What would you recommend else? Introduce another syntax for selectors?

Something like "make file-notify-tests TEST=06", for example.

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> That's your logic, not mine.  If I want all the tests, I will say so.

How?

>> What would you recommend else? Introduce another syntax for selectors?
>
> Something like "make file-notify-tests TEST=06", for example.

The "06" is just my invention. There's no rule how to name ert
tests. Most of the test names do not use numbers.

And a selector '(eql file-notify-test02-events) is just one of the
possible selectors. Do you want to restrict this to test names? Using
tags for selectors sounds also very appealing.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: fabrice.popineau@gmail.com,  22534@debbugs.gnu.org
> Date: Mon, 08 Feb 2016 22:13:02 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > That's your logic, not mine.  If I want all the tests, I will say so.
> 
> How?

By the same selector scheme we use when running the whole suite.

> >> What would you recommend else? Introduce another syntax for selectors?
> >
> > Something like "make file-notify-tests TEST=06", for example.
> 
> The "06" is just my invention. There's no rule how to name ert
> tests. Most of the test names do not use numbers.
> 
> And a selector '(eql file-notify-test02-events) is just one of the
> possible selectors. Do you want to restrict this to test names? Using
> tags for selectors sounds also very appealing.

The name of a test is a good selector, and the syntax could be simpler
than it is now, if we could say something like "TESTS='foo bar baz'".

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> > That's your logic, not mine.  If I want all the tests, I will say so.
>>
>> How?
>
> By the same selector scheme we use when running the whole suite.

Well, the following runs all tests but the expensive ones:

# make SELECTOR='$(SELECTOR_DEFAULT)' -C test/automated file-notify-tests

So far, we have declared only $(SELECTOR_DEFAULT) and
$(SELECTOR_EXPENSIVE). Shall we declare more predefined selectors in
the makefile?

>> > Something like "make file-notify-tests TEST=06", for example.
>>
>> The "06" is just my invention. There's no rule how to name ert
>> tests. Most of the test names do not use numbers.
>>
>> And a selector '(eql file-notify-test02-events) is just one of the
>> possible selectors. Do you want to restrict this to test names? Using
>> tags for selectors sounds also very appealing.
>
> The name of a test is a good selector, and the syntax could be simpler
> than it is now, if we could say something like "TESTS='foo bar baz'".

Selectors could be a regexp. So the following works already:

# make SELECTOR='\"02\"' -C test/automated file-notify-tests

And if you want to run only test 04 and 05 (both about validity), and
not the respective remote tests, call this (note the double $ at the end):

# make SELECTOR='\"validity$$\"' -C test/automated file-notify-tests

Well, I shall start to document somewhere what's already working ...

Best regards, Michael.

bug#22534: File notify broken on Windows

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

> Well, I shall start to document somewhere what's already working ...

I've added some examples to CONTRIBUTE.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: fabrice.popineau@gmail.com,  22534@debbugs.gnu.org
> Date: Tue, 09 Feb 2016 11:08:29 +0100
> 
> Michael Albinus <michael.albinus@gmx.de> writes:
> 
> > Well, I shall start to document somewhere what's already working ...
> 
> I've added some examples to CONTRIBUTE.

Thanks, but I'd rather have them in test/README.  CONTRIBUTE is too
far away from the tests to believe that interested users will look
there (but I don't object to having them there as well, if you'd like
that).

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but I'd rather have them in test/README.  CONTRIBUTE is too
> far away from the tests to believe that interested users will look
> there (but I don't object to having them there as well, if you'd like
> that).

Yes, test/README looks very short. I'll move most of the text about
testing from CONTRIBUTE to test/README. Likely, tomorrow.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

> Yes, test/README looks very short. I'll move most of the text about
> testing from CONTRIBUTE to test/README. Likely, tomorrow.

Done.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

2016-02-08 19:09 GMT+01:00 Eli Zaretskii <eliz@gnu.org>:

> > From: Fabrice Popineau <fabrice.popineau@gmail.com>
> > Date: Mon, 8 Feb 2016 11:43:10 +0100
> > Cc: Eli Zaretskii <eliz@gnu.org>, 22534@debbugs.gnu.org
> >
> >  > events: ((created changed deleted)) ((17491048 created
> >  >
> c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo)
> >  > (17491048 changed
> >  >
> c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo))
> >
> >  Fails also for me in batch mode. Surprisingly, it doesn't fail always,
> >  sometimes (about 1 in 10 runs) it succeeds here, and fails in the next
> >  step. Strange.
> >
> > I see the same. The deleted notifications number vary. When you expect
> 2, sometimes I see 1.
> > When you expect one, sometimes I see it. But not frequently.
>
> Did you try adding calls to read-event after the functions that are
> supposed to produce these events?
>
> Yes, but with no results.

[-- Attachment #2: Type: text/html, Size: 1607 bytes --]

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Mon, 8 Feb 2016 20:15:59 +0100
> Cc: Michael Albinus <michael.albinus@gmx.de>, 22534@debbugs.gnu.org
> 
>  > From: Fabrice Popineau <fabrice.popineau@gmail.com>
>  > Date: Mon, 8 Feb 2016 11:43:10 +0100
>  > Cc: Eli Zaretskii <eliz@gnu.org>, 22534@debbugs.gnu.org
>  >
>  > > events: ((created changed deleted)) ((17491048 created
>  > > c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo)
>  > > (17491048 changed
>  > > c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo))
>  >
>  > Fails also for me in batch mode. Surprisingly, it doesn't fail always,
>  > sometimes (about 1 in 10 runs) it succeeds here, and fails in the next
>  > step. Strange.
>  >
>  > I see the same. The deleted notifications number vary. When you expect 2, sometimes I see 1.
>  > When you expect one, sometimes I see it. But not frequently.
> 
>  Did you try adding calls to read-event after the functions that are
>  supposed to produce these events?
> 
> Yes, but with no results.

Strange.  I see none of that on my machine.  Maybe it's because my
disk is SSD, maybe because this is XP.  Who knows?

Michael, I think it's time to do what you suggested:

> If we do not care the missing `deleted' event in batch mode, I could
> make it optional in the test.

Re this:

> But maybe it is an indication for a serious problem in w32notify.

It might, but it's unlikely, and I won't have time to dig into this in
the following weeks, so I guess this will have to wait for a recipe I
can reproduce on one of my machines.

Thanks.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 7998 bytes --]

Some information regarding the original problem.

I have turned on some tracing and this is what I get with
file-notify-test02-events
along these lines:

$ git diff -w origin/emacs-25 src/w32notify.c
diff --git a/src/w32notify.c b/src/w32notify.c
index 71787c4..66ad277 100644
--- a/src/w32notify.c
+++ b/src/w32notify.c
@@ -207,6 +207,7 @@ watch_completion (DWORD status, DWORD bytes_ret,
OVERLAPPED *io_info)
      freed by someone already?  In any case, we cannot do anything
      with this request, so just punt and skip it.  FIXME: should we
      raise the 'terminate' flag in this case?  */
+  DebPrint(("status = %lx, io_info = %lx\n", status, io_info));
   if (!io_info)
     return;

@@ -227,6 +228,7 @@ watch_completion (DWORD status, DWORD bytes_ret,
OVERLAPPED *io_info)
          by its buffers; this is done by the main thread in
          remove_watch.  Calling malloc/free from a thread other than
          the main thread is a no-no.  */
+      DebPrint(("Operation aborted."));
       dirwatch->dir = NULL;
       dirwatch->terminate = 1;
     }
@@ -406,11 +408,13 @@ remove_watch (struct notification *dirwatch)
             break;
           Sleep (10);
        }
+      DebPrint(("status = %lx, exit_code = %lx\n", status, exit_code));
       if ((status == FALSE && (err = GetLastError ()) ==
ERROR_INVALID_HANDLE)
           || exit_code == STILL_ACTIVE)
        {
           if (!(status == FALSE && err == ERROR_INVALID_HANDLE))
             {
+              DebPrint(("Terminating thread.\n"));
               TerminateThread (dirwatch->thr, 0);
               if (dirwatch->dir)
                CloseHandle (dirwatch->dir);
@@ -523,7 +527,7 @@ generate notifications correctly, though.  */)
   char *errstr;

   CHECK_LIST (filter);
-
+  DebPrint(("AddWatch\n"));
   /* The underlying features are available only since XP.  */
   if (os_subtype == OS_9X
       || (w32_major_version == 5 && w32_minor_version < 1))

I also forced DebPrint to write on stderr.


When run in batch mode:

$ make SELECTOR='(quote (eql file-notify-test02-events))'   -C
test/automated file-notify-tests
make : on entre dans le répertoire «
/d/Source/emacs/build-emacs-25/test/automated »
make[1] : on entre dans le répertoire «
/d/Source/emacs/build-emacs-25/test/automated »
make[2] : on entre dans le répertoire «
/d/Source/emacs/build-emacs-25/test/automated »
Compiling ../../../emacs/test/automated/file-notify-tests.el
make[2] : on quitte le répertoire «
/d/Source/emacs/build-emacs-25/test/automated »
Testing ../../../emacs/test/automated/file-notify-tests.elc
Running 1 tests (2016-02-11 16:14:39+0100)
AddWatch
status = 0, io_info = 3dd8090
status = 0, io_info = 3dd8090
status = 0, io_info = 3dd8090
status = 3e3, io_info = 3dd8090
Operation aborted.
status = 1, exit_code = 0
AddWatch
status = 0, io_info = 3dd8210
status = 0, io_info = 3dd8210
status = 0, io_info = 3dd8210
status = 3e3, io_info = 3dd8210
Operation aborted.
status = 1, exit_code = 0
AddWatch
status = 0, io_info = 3dd8000
status = 0, io_info = 3dd8000
watch_worker, abnormal exit: 5
QueueUserAPC failed (31)!
status = 1, exit_code = 1
Test file-notify-test02-events backtrace:
  #[0 "\306\307\310C\307C\3111(\312\313\314\315$\317\"\32
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test file-notify-test02-events "Check fi
  ert-run-or-rerun-test([cl-struct-ert--stats (eql file-notify-test02-
  ert-run-tests((eql file-notify-test02-events) #[385 "\306\307\"\203
  ert-run-tests-batch((eql file-notify-test02-events))
  ert-run-tests-batch-and-exit((eql file-notify-test02-events))
  eval((ert-run-tests-batch-and-exit (quote (eql file-notify-test02-ev
  command-line-1(("-L" ";../../../emacs/test/automated" "-l" "ert" "-l
  command-line()
  normal-top-level()
Test file-notify-test02-events condition:
    (ert-test-failed
     ((should
       (file-notify--test-with-events-check events))
      :form
      (file-notify--test-with-events-check
       ((created changed deleted)))
      :value nil :explanation "Received events `(created changed)' do not
match expected events `(created changed deleted)'"))
   FAILED  1/1  file-notify-test02-events

Ran 1 tests, 0 results as expected, 1 unexpected (2016-02-11 16:15:20+0100)

1 unexpected results:
   FAILED  file-notify-test02-events

When I run in normal mode:

$ src/emacs -Q -l ../emacs/test/automated/file-notify-tests.el
AddWatch
status = 0, io_info = 42dd0a0
status = 0, io_info = 42dd0a0
status = 0, io_info = 42dd0a0
status = 0, io_info = 42dd0a0
status = 3e3, io_info = 42dd0a0
Operation aborted.status = 1, exit_code = 0
AddWatch
status = 0, io_info = 42dcc50
status = 0, io_info = 42dcc50
status = 0, io_info = 42dcc50
status = 0, io_info = 42dcc50
status = 3e3, io_info = 42dcc50
Operation aborted.status = 1, exit_code = 0
AddWatch
status = 0, io_info = 42dd310
status = 0, io_info = 42dd310
status = 0, io_info = 42dd310
watch_worker, abnormal exit: 5
QueueUserAPC failed (31)!
status = 1, exit_code = 1
AddWatch
status = 0, io_info = 42dd2b0
status = 0, io_info = 42dd2b0
status = 0, io_info = 42dd2b0
status = 0, io_info = 42dd2b0
status = 0, io_info = 42dd2b0
status = 0, io_info = 42dd2b0
status = 0, io_info = 42dd2b0
status = 0, io_info = 42dd2b0
watch_worker, abnormal exit: 5
QueueUserAPC failed (31)!
status = 1, exit_code = 1
AddWatch
status = 0, io_info = 42dcec0
status = 0, io_info = 42dcec0
status = 0, io_info = 42dcec0
status = 0, io_info = 42dcec0
status = 5, io_info = 42dcec0
watch_worker, abnormal exit: 5
QueueUserAPC failed (31)!
status = 1, exit_code = 1
AddWatch
status = 0, io_info = 42dcec0
status = 0, io_info = 42dcec0
status = 3e3, io_info = 42dcec0
Operation aborted.status = 1, exit_code = 0

and the test is passed.


The first diagnostic is that QueueUserAPC shouldn't fail with error 31.
This happens because the worker thread is already terminating, but that
does not leave
a chance for watch_end to run.

Fabrice

2016-02-10 20:15 GMT+01:00 Eli Zaretskii <eliz@gnu.org>:

> > From: Fabrice Popineau <fabrice.popineau@gmail.com>
> > Date: Mon, 8 Feb 2016 20:15:59 +0100
> > Cc: Michael Albinus <michael.albinus@gmx.de>, 22534@debbugs.gnu.org
> >
> >  > From: Fabrice Popineau <fabrice.popineau@gmail.com>
> >  > Date: Mon, 8 Feb 2016 11:43:10 +0100
> >  > Cc: Eli Zaretskii <eliz@gnu.org>, 22534@debbugs.gnu.org
> >  >
> >  > > events: ((created changed deleted)) ((17491048 created
> >  > >
> c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo)
> >  > > (17491048 changed
> >  > >
> c:/Users/Fabrice/AppData/Local/Temp/file-notify-test-parent20748Ali/file-notify-test20748Nvo))
> >  >
> >  > Fails also for me in batch mode. Surprisingly, it doesn't fail always,
> >  > sometimes (about 1 in 10 runs) it succeeds here, and fails in the next
> >  > step. Strange.
> >  >
> >  > I see the same. The deleted notifications number vary. When you
> expect 2, sometimes I see 1.
> >  > When you expect one, sometimes I see it. But not frequently.
> >
> >  Did you try adding calls to read-event after the functions that are
> >  supposed to produce these events?
> >
> > Yes, but with no results.
>
> Strange.  I see none of that on my machine.  Maybe it's because my
> disk is SSD, maybe because this is XP.  Who knows?
>
> Michael, I think it's time to do what you suggested:
>
> > If we do not care the missing `deleted' event in batch mode, I could
> > make it optional in the test.
>
> Re this:
>
> > But maybe it is an indication for a serious problem in w32notify.
>
> It might, but it's unlikely, and I won't have time to dig into this in
> the following weeks, so I guess this will have to wait for a recipe I
> can reproduce on one of my machines.
>
> Thanks.
>

[-- Attachment #2: Type: text/html, Size: 10895 bytes --]

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Thu, 11 Feb 2016 16:19:49 +0100
> Cc: Michael Albinus <michael.albinus@gmx.de>, 22534@debbugs.gnu.org
> 
> Some information regarding the original problem.

Thanks.

> The first diagnostic is that QueueUserAPC shouldn't fail with error 31.
> This happens because the worker thread is already terminating, but that does not leave
> a chance for watch_end to run.

Aren't those the cases when the parent directory of the files being
watched is deleted?  If so, do you see how to prevent the thread from
terminating abnormally?

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli & Fabrice,

> Michael, I think it's time to do what you suggested:
>
>> If we do not care the missing `deleted' event in batch mode, I could
>> make it optional in the test.

I'm still undecided. Are there chances there'll be progress before the
release? Then I won't touch it.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Fabrice Popineau <fabrice.popineau@gmail.com>,  22534@debbugs.gnu.org
> Date: Mon, 15 Feb 2016 16:31:00 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> Hi Eli & Fabrice,
> 
> > Michael, I think it's time to do what you suggested:
> >
> >> If we do not care the missing `deleted' event in batch mode, I could
> >> make it optional in the test.
> 
> I'm still undecided. Are there chances there'll be progress before the
> release?

Not by me, as long as I don't see the problem on my machine.  If
Fabrice could find time debugging this, I will be glad to help by
explaining the code in w32notify.c and elsewhere that is related to
injecting the notification events on w32.

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Not by me, as long as I don't see the problem on my machine.  If
> Fabrice could find time debugging this, I will be glad to help by
> explaining the code in w32notify.c and elsewhere that is related to
> injecting the notification events on w32.

So I let this decision to Fabrice.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

2016-02-15 18:52 GMT+01:00 Michael Albinus <michael.albinus@gmx.de>:

> Eli Zaretskii <eliz@gnu.org> writes:
>
> > Not by me, as long as I don't see the problem on my machine.  If
> > Fabrice could find time debugging this, I will be glad to help by
> > explaining the code in w32notify.c and elsewhere that is related to
> > injecting the notification events on w32.
>
> So I let this decision to Fabrice.
>
>
I will return to my previous investigation of this problem (after bug 22526
interruption)
Can't guarantee that I can come with something in a short time however

Fabrice

[-- Attachment #2: Type: text/html, Size: 1052 bytes --]

bug#22534: File notify broken on Windows

[Michael Albinus]

Fabrice Popineau <fabrice.popineau@gmail.com> writes:

> In the 'check for attribute change', I see only 2 'changed'
> notifications, and not 4
> as expected by the test (around line 512: w32notify does not
> distinguish between 
> 'changed' and 'attribute-changed').

I cannot confirm this, it works for me. I'm running the test on a
Windows 7 machine using Emacs build "GNU Emacs 25.0.50.1
(i686-pc-mingw32) of 2016-01-29", the one provided by Dani Moncayo, plus
the updated filenotify.el and file-notify-tests.el.

Do you run Windows 10? Something else different?

> Fabrice

Best regards, Michael.

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

2016-02-08 11:21 GMT+01:00 Michael Albinus <michael.albinus@gmx.de>:

> Fabrice Popineau <fabrice.popineau@gmail.com> writes:
>
> > In the 'check for attribute change', I see only 2 'changed'
> > notifications, and not 4
> > as expected by the test (around line 512: w32notify does not
> > distinguish between
> > 'changed' and 'attribute-changed').
>
> I cannot confirm this, it works for me. I'm running the test on a
> Windows 7 machine using Emacs build "GNU Emacs 25.0.50.1
> (i686-pc-mingw32) of 2016-01-29", the one provided by Dani Moncayo, plus
> the updated filenotify.el and file-notify-tests.el.
>
>
Ok, I may have been mistaken. At the moment, I see the 4 notifications.


> Do you run Windows 10? Something else different?
>
>
Windows 10, and I compile with MSys2-MinGW64.


Fabrice

[-- Attachment #2: Type: text/html, Size: 1634 bytes --]

bug#22534: File notify broken on Windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

2016-02-08 11:40 GMT+01:00 Fabrice Popineau <fabrice.popineau@gmail.com>:

>
>
> 2016-02-08 11:21 GMT+01:00 Michael Albinus <michael.albinus@gmx.de>:
>
>> Fabrice Popineau <fabrice.popineau@gmail.com> writes:
>>
>> > In the 'check for attribute change', I see only 2 'changed'
>> > notifications, and not 4
>> > as expected by the test (around line 512: w32notify does not
>> > distinguish between
>> > 'changed' and 'attribute-changed').
>>
>> I cannot confirm this, it works for me. I'm running the test on a
>> Windows 7 machine using Emacs build "GNU Emacs 25.0.50.1
>> (i686-pc-mingw32) of 2016-01-29", the one provided by Dani Moncayo, plus
>> the updated filenotify.el and file-notify-tests.el.
>>
>>
> Ok, I may have been mistaken. At the moment, I see the 4 notifications.
>

After a couple of test runs, I can confirm that sometimes, only 2
notifications are reported
in batch mode. Same problem as with the deleted events.

Fabrice

[-- Attachment #2: Type: text/html, Size: 1883 bytes --]

bug#22534: File notify broken on Windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Fri, 5 Feb 2016 16:34:17 +0100
> Cc: Michael Albinus <michael.albinus@gmx.de>, 22534@debbugs.gnu.org
> 
>  Hmm... actually, I might see a problem. If my reading of the test is
>  correct, it first creates 2000 files, and then issues 1000 renames,
>  which on Windows generate 2000 notifications. Together, these add up
>  to 4000, which is awfully close to the 4096 size of the Emacs input
>  event queue. So maybe the test fills up the event queue, and the
>  synchronization between the w32notify thread and the main thread stops
>  working at that point? Or maybe we are hitting on some limit of
>  Windows messages that can be enqueued?
> 
> I would have expected some Win32 function to error at some point ?

Maybe it does, and we don't check error codes?

But these are speculations.  And since all my other guesses were
wrong, this one most probably is as well.  I will look into this later
if no one beats me to it.

> Also, would it be useful to use the overlap mechanism?

Where? in reading Windows messages?

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Ok. Maybe that's the reason, but in this case, the expected events
>> have to be adjusted because 'make check'
>> runs in batch mode.
>
> True.  I have done that several times to fix that test; maybe it's
> time to do that again.

For the records, I'm not able to build Emacs by myself under MS Windows;
I use the distribution of Dani Moncayo. It even does not contain the
test directory; I must arrange it by myself.

I'm grateful if you guys could run the batch tests. And maybe propose
something more clever to wait for events in such an environment.

> Thanks.

Best regards, Michael.

bug#22534: File notify broken on Windows

[Michael Albinus]

Fabrice Popineau <fabrice.popineau@gmail.com> writes:

[Added 22534@debbugs.gnu.org to Cc]

> Ok, patch attached.
>
> The remaining DebPrint() should be left in case of problems.
> They all indicate some sort of error (I think).
>
> I have a disagreement on this test:
>
> diff --git a/test/lisp/filenotify-tests.el
> b/test/lisp/filenotify-tests.el
> index bc64f86..20e01fc 100644
> --- a/test/lisp/filenotify-tests.el
> +++ b/test/lisp/filenotify-tests.el
> @@ -530,7 +530,7 @@ file-notify--test-with-events
> ;; w32notify does not distinguish between `changed' and
> ;; `attribute-changed'.
> ((string-equal (file-notify--test-library) "w32notify")
> - '(changed changed changed changed))
> + '(changed changed))
> ;; For kqueue and in the remote case, `write-region'
> ;; raises also an `attribute-changed' event.
> ((or (string-equal (file-notify--test-library) "kqueue")
>
> At least on Windows 10, I get only 2 notifications and not 4.
> Could you or Michael check about it?

As soon as I get a compiled Emacs for w32, including your patch, I'll
test it. It will be on a Windows 7 machine, hope there are no
differences to your test.

> Best regards,

Best regards, Michael.

bug#22534: File notify broken on Windows

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> New patch attached against master. To mirror malloc(), free() is
>> called instead
>> of xfree().
>
> Thanks, pushed.

Thanks to both of you. I'm closing the bug.

Best regards, Michael.