Created attachment 217889 [details] [review]
win32: user overlapped events for streams

Any file handle created with FLAG_OVERLAPPED must have
ReadFile()/WriteFile() called with an OVERLAPPED structure.
Failing to do so will give unspecified results, invalid read/write or
corruption.

Without FLAG_OVERLAPPED, it is not possible to read and write
concurrently, even with two seperate threads, created by 2 input and
output gio streams. Also, only with FLAG_OVERLAPPED may an IO
operation be asynchronous and thus be cancellable.

We may want to call ReOpenFile() to make sure the FLAG is set, but
this API is only available since Vista+.

According to MSDN doc, adding the OVERLAPPED argument for IO operation
on handles without FLAG_OVERLAPPED is allowed, and indeed the existing
test still passes.

Created attachment 217890 [details] [review]
win32: add pipe-io-cancel-test

Test that win32 streams can be cancelled.

Tor, you wrote that code before, with this comment in log:

    Correspond to GUnixInputStream and GUnixOutputStream. No true async
    support though. But that is how the Win32 API is, for files not
    explicitly opened for so-called overlapped IO.

If you could comment this patch, that would be nice. After a lot of sweat, I realized that:

- a file created without FLAG_OVERLAPPED may still take overlapped argument but return immediately in this case (the change here handles both immediate return and wait cases)

- without FLAG_OVERLAPPED, corruption or invalid IO may happen if simultaneous R/W, which is something I couldn't find commented in GIO code, or in commit messages (took me a while to realize).

- without FLAG_OVERLAPPED, we can't (cleanly) cancel a pending IO

hopefully I am not wrong :)

Created attachment 217952 [details] [review]
win32: add pipe-io-cancel-test

Test that win32 streams can be cancelled.

Created attachment 217953 [details] [review]
win32: user overlapped events for streams

Any file handle created with FLAG_OVERLAPPED must have
ReadFile()/WriteFile() called with an OVERLAPPED structure.
Failing to do so will give unspecified results, invalid read/write or
corruption.

Without FLAG_OVERLAPPED, it is not possible to read and write
concurrently, even with two seperate threads, created by 2 input and
output gio streams. Also, only with FLAG_OVERLAPPED may an IO
operation be asynchronous and thus be cancellable.

We may want to call ReOpenFile() to make sure the FLAG is set, but
this API is only available since Vista+.

According to MSDN doc, adding the OVERLAPPED argument for IO operation
on handles without FLAG_OVERLAPPED is allowed, and indeed the existing
test still passes.

Created attachment 217954 [details] [review]
win32: add pipe-io-cancel-test

Test that win32 streams can be cancelled.

last update just fix some coding style

So concretely this patch is adding cancellability for streams that applications are creating with FLAG_OVERLAPPED manually?

In other words, this is not a bugfix, but a new feature?

(In reply to comment #8)
> So concretely this patch is adding cancellability for streams that applications
> are creating with FLAG_OVERLAPPED manually?

correct
 
> In other words, this is not a bugfix, but a new feature?

You could say so, but given that it wasn't explicit or checked whether the stream had to be !FLAG_OVERLAPPED in order to do concurrent r/w, I would say it should have worked, and not give completely unpredictable results. So, it's a fix for me, if you consider that it wasn't obvious it wouldn't support that case.

What I could do is to make the cancellable part separately, if needed. It's quite minimal and obvious looking at the _g_win32_overlap_wait_result() how it is done. The code hints what is done when cancellable is disabled, when it is not pollable.

(In reply to comment #9)
> (In reply to comment #8)
> > So concretely this patch is adding cancellability for streams that applications
> > are creating with FLAG_OVERLAPPED manually?
> 
> correct

What I was getting at is that the title "win32: user overlapped events for streams" is really non-descriptive.  Can we change it to something like:

"win32: Allow cancelling application-created FLAG_OVERLAPPED streams"

> > In other words, this is not a bugfix, but a new feature?
> 
> You could say so, but given that it wasn't explicit or checked whether the
> stream had to be !FLAG_OVERLAPPED in order to do concurrent r/w, I would say it
> should have worked, and not give completely unpredictable results. So, it's a
> fix for me, if you consider that it wasn't obvious it wouldn't support that
> case.

Fair enough.  Anyways this needs review from a win32 person; danw?  Or maybe you'll succeed in extracting Tor from Android/Libreoffice or wherever he is now =)

(In reply to comment #10)

> What I was getting at is that the title "win32: user overlapped events for
> streams" is really non-descriptive.  Can we change it to something like:
> 
> "win32: Allow cancelling application-created FLAG_OVERLAPPED streams"
> 

Well, not only. The main advantage is not the added cancel, it's the fact that the code is using OVERLAPPED event if possible, and thus fixing invalid calls when doing concurrent r/w. I was fixing some corrupted and invalid return value before, and this patch solves the corruption (the main point of the patch imho).

A further patch that would be for Vista+ could make sure the handle is using FLAG_OVERLAPPED.

As I said, the cancellation could be made a seperate patch.

(In reply to comment #10)
> Fair enough.  Anyways this needs review from a win32 person; danw?

I'm not a win32 person, I just fake it sometimes (and mostly only in the network API space). I'm aware of the existence of "overlapped I/O", but don't know enough about it to say whether or not this patch uses it correctly, or if that's the right fix, etc.

But hey, test cases!

(Actually, it would be good to have a test case demonstrating that simultaneous read/write worked too. Via both sync and async APIs...)

also, I assume "user" in the commit message / bug summary is a typo for "use"?

Created attachment 218133 [details] [review]
win32: add pipe-io-cancel-test

Test that win32 streams can be cancelled.
It can even be tested with wine on Linux!

Created attachment 218134 [details] [review]
win32: add pipe-io-cancel-test

Test that win32 streams can be cancelled.
It can even be tested with wine on Linux!

Created attachment 218135 [details] [review]
win32: add pipe-io-overlap-test

Note that wine doesn't handle that test for some reason. On windows,
it fails in various ways, it's racy, buggy.

Most probably, what seems to happen, is that one of the concurrent
Read/Write will complete, and the other will return with either a read
of 0, or an invalid buffer.

(truth be said, a read of 0 isn't necessarily an eof or invalid read
for a namedpipe, if the other end of the pipe did a Write(0) it is a
normal condition, which isn't possible to distinguish atm with GIO
api, since it maps to EOF or BROKEN_PIPE, but that's a different
problem anyway)

Created attachment 218136 [details] [review]
win32: use overlapped events for streams

Any file handle created with FLAG_OVERLAPPED must have
ReadFile()/WriteFile() called with an OVERLAPPED structure.
Failing to do so will give unspecified results, invalid read/write or
corruption.

Without FLAG_OVERLAPPED, it is not possible to read and write
concurrently, even with two seperate threads, created by 2 input and
output gio streams. Also, only with FLAG_OVERLAPPED may an IO
operation be asynchronous and thus be cancellable.

We may want to call ReOpenFile() to make sure the FLAG is set, but
this API is only available since Vista+.

According to MSDN doc, adding the OVERLAPPED argument for IO operation
on handles without FLAG_OVERLAPPED is allowed, and indeed the existing
test still passes.

Created attachment 218137 [details] [review]
win32: make gio stream cancellable

ping :)

Review of attachment 218136 [details] [review]:

::: gio/gwin32inputstream.c
@@ +323,3 @@
+        }
+
+      if (g_cancellable_set_error_if_cancelled (cancellable, error))

This will be hit if the function above returns FALSE (which it will in the case that the cancellable has been cancelled.  The problem is that it may have been cancelled and then reset by the time that it gets here...

Probably _g_win32_overlap_wait_result() should return its own GError?

@@ +326,3 @@
+        goto end;
+
+      if (errsv == ERROR_MORE_DATA)

In the case that we did a read from a socket (resulting in ERROR_IO_PENDING) and then the socket was closed by the remote, we will get an ERROR_HANDLE_EOF in GetLastError() but errsv is still set from before, so will still be ERROR_IO_PENDING...

Also: it looks like the additional handling of an error case here is unrelated to the rest of this patch...

Review of attachment 218137 [details] [review]:

::: gio/gasynchelper.c
@@ +196,3 @@
+    /* CancelIoEx is only Vista+. Since we have only one overlap
+     * operaton on this thread, we can just use: */
+    g_assert (CancelIo (hfile));

g_assert() should not be used like this because it can be optimised away...

Please change the comment to note that this is actually OK: CancelIO only cancels pending operations issued by the current thread and since we're doing only sync operations, this is safe....

@@ +200,2 @@
   /* either cancelled or IO completed, either way get the result */
   result = GetOverlappedResult (overlap->hEvent, overlap, transferred, TRUE);

There is a potential race here that's really unlikely to happen, but here we go:

We are trying to read from the same socket in two threads.  Some data comes.  That causes the poll() in both threads (above) to finish running.  Then the cancellable is checked above.

We now find ourselves here.  Only one thread will read the data.  The other will block on this function.  Then the user may cancel the cancellable while we are blocked here, but we will stay blocked....

(In reply to comment #20)
> This will be hit if the function above returns FALSE (which it will in the case
> that the cancellable has been cancelled.  The problem is that it may have been
> cancelled and then reset by the time that it gets here...

No, that would be a programmer error:

/**
 * g_cancellable_reset:
 * ...
 * If cancellable is currently in use by any cancellable operation
 * then the behavior of this function is undefined.
 **/

Review of attachment 218137 [details] [review]:

::: gio/gasynchelper.c
@@ +196,3 @@
+    /* CancelIoEx is only Vista+. Since we have only one overlap
+     * operaton on this thread, we can just use: */
+    g_assert (CancelIo (hfile));

the g_assert() here is actually only useful for debugging, and it's fine if it's compiled away. The alternative would be to write a warning() I guess?

I hinted that call was fine since we have only one overlap operation in that thread:
"Since we have only one overlap operaton on this thread, we can just use"

I added your comment too.

@@ +200,2 @@
   /* either cancelled or IO completed, either way get the result */
   result = GetOverlappedResult (overlap->hEvent, overlap, transferred, TRUE);

I added a test case for this.

The wait argument of GetOverlappedResult() can be set to FALSE, and in case we get ERROR_IO_INCOMPLETE error and operation is not cancelled, then loop again.

Created attachment 220614 [details] [review]
win32: add pipe-io-cancel-test

Test that win32 streams can be cancelled.
It can even be tested with wine on Linux!

Created attachment 220615 [details] [review]
win32: add pipe-io-overlap-test

Note that wine doesn't handle that test for some reason. On windows,
it fails in various ways, it's racy, buggy.

Most probably, what seems to happen, is that one of the concurrent
Read/Write will complete, and the other will return with either a read
of 0, or an invalid buffer.

(truth be said, a read of 0 isn't necessarily an eof or invalid read
for a namedpipe, if the other end of the pipe did a Write(0) it is a
normal condition, which isn't possible to distinguish atm with GIO
api, since it maps to EOF or BROKEN_PIPE, but that's a different
problem anyway)

Created attachment 220616 [details] [review]
win32: add pipe-io-concurrent

Implement test case suggested by Ryan Lortie on bug:
https://bugzilla.gnome.org/show_bug.cgi?id=679288

"There is a potential race here that's really unlikely to happen, but
here we go: We are trying to read from the same socket in two threads.
Some data comes. That causes the poll() in both threads (above) to
finish running. Then the cancellable is checked above. We now find
ourselves here. Only one thread will read the data. The other will
block on this function. Then the user may cancel the cancellable while
we are blocked here, but we will stay blocked...."

Created attachment 220617 [details] [review]
win32: use overlapped events for streams

Any file handle created with FLAG_OVERLAPPED must have
ReadFile()/WriteFile() called with an OVERLAPPED structure.
Failing to do so will give unspecified results, invalid read/write or
corruption.

Without FLAG_OVERLAPPED, it is not possible to read and write
concurrently, even with two seperate threads, created by 2 input and
output gio streams. Also, only with FLAG_OVERLAPPED may an IO
operation be asynchronous and thus be cancellable.

We may want to call ReOpenFile() to make sure the FLAG is set, but
this API is only available since Vista+.

According to MSDN doc, adding the OVERLAPPED argument for IO operation
on handles without FLAG_OVERLAPPED is allowed, and indeed the existing
test still passes.

v2:
- update GetLastError() after _g_win32_overlap_wait_result ()
- split the unrelated ERROR_MORE_DATA handling

Created attachment 220618 [details] [review]
win32: handle ERROR_MORE_DATA

If a named pipe is being read in message mode and the next message is
longer than the nNumberOfBytesToRead parameter specifies, ReadFile
returns FALSE and GetLastError returns ERROR_MORE_DATA.

Since the API doesn't allow to return both a GError and the number of
bytes read so far, it makes more sense to return nread, and let the
client call GetLastError() himself to check if ERROR_MORE_DATA.

The current alternative loses the nread information.

Created attachment 220619 [details] [review]
win32: make gio stream cancellable

You misunderstand my comment about the g_assert().

When you write g_assert(x) and it gets optimised away, it does not get replaced by 'x'.  It gets replaced by nothing.  ie: the CancelIo() call can disappear entirely.

Created attachment 220654 [details] [review]
win32: make gio stream cancellable

Review of attachment 220654 [details] [review]:

::: gio/gasynchelper.c
@@ +187,3 @@
+loop:
+  if (g_cancellable_make_pollfd (cancellable, &pollfd[1]))
+    num++;

It looks like if we loop back (in the race case) then num will continue to grow...

Probably we also end up leaking the pollfd as well?

Created attachment 220687 [details] [review]
win32: make gio stream cancellable

v2:
 - fix cancellation of concurrent readers
 - replace g_assert() usage with g_warn_if_fail()
v3:
 - fix indentation
 - fix loop code to not leak (silly me)

Comment on attachment 220614 [details] [review]
win32: add pipe-io-cancel-test

Wait, doesn't test_pipe_io() already test cancellation? What's the point of all the cancellables and the timeout otherwise?

>+  g_assert (err != NULL);

FWIW it would be better to do

  g_assert_error (err, G_IO_ERROR, G_IO_ERROR_CANCELLED);

(In reply to comment #34)
> (From update of attachment 220614 [details] [review])
> Wait, doesn't test_pipe_io() already test cancellation? What's the point of all
> the cancellables and the timeout otherwise?

To cancel after some IO completed (even if it passes the cancellable around), the handles are not opened with FILE_FLAG_OVERLAPPED even.

> >+  g_assert (err != NULL);
> 
> FWIW it would be better to do
> 
>   g_assert_error (err, G_IO_ERROR, G_IO_ERROR_CANCELLED);

ack

Created attachment 220700 [details] [review]
win32: add pipe-io-cancel-test

Test that win32 streams can be cancelled.
It can even be tested with wine on Linux!

no objections to the last round of patches? thanks

I'm deeply unfamiliar with Windows but I don't have any objection to the latest patches.  If you're sure this is right and Dan wants to ACK it, I'm happy.

Comment on attachment 220615 [details] [review]
win32: add pipe-io-overlap-test

>On windows,
>it fails in various ways, it's racy, buggy.

I assume you mean it fails without your patch, but succeeds with it? If so, just commit the test case after the patch that fixes it. If not, then we don't want this test, because tests should always pass.

>(truth be said, a read of 0 isn't necessarily an eof or invalid read
>for a namedpipe, if the other end of the pipe did a Write(0) it is a
>normal condition, which isn't possible to distinguish atm with GIO
>api, since it maps to EOF or BROKEN_PIPE, but that's a different
>problem anyway)

this doesn't really belong in the commit message

Comment on attachment 220617 [details] [review]
win32: use overlapped events for streams

>+_g_win32_overlap_wait_result (HANDLE           hfile,
>+                              OVERLAPPED      *overlap,
>+                              DWORD           *transferred,
>+                              GCancellable    *cancellable G_GNUC_UNUSED)
>+{
>+  GPollFD pollfd[1] = { 0, };

is there any reason to use g_poll() here rather than just calling the underlying windows method directly? (MsgWaitForMultipleObjects() or whatever)

>+  overlap.hEvent = CreateEvent (NULL, FALSE, FALSE, NULL);

would it make more sense to have a single hEvent for the stream and reuse it every time, rather than creating a new one for every read?

>@@ -304,24 +306,49 @@ g_win32_output_stream_write (GOutputStream  *stream,

>+      if (errsv == ERROR_HANDLE_EOF ||
>+          errsv == ERROR_BROKEN_PIPE)
>+        {
>+          retval = 0;
>+        }

This is wrong (but it's wrong in the existing code too); if you get an error on write, you should just return the error, not "0".

Comment on attachment 220618 [details] [review]
win32: handle ERROR_MORE_DATA

i guess this makes sense...

Comment on attachment 220687 [details] [review]
win32: make gio stream cancellable

>+  if (g_cancellable_make_pollfd (cancellable, &pollfd[1]))
>+    num++;

ah, that answers my previous question...

this looks right I guess

(In reply to comment #39)
> (From update of attachment 220615 [details] [review])
> >On windows,
> >it fails in various ways, it's racy, buggy.
> 
> I assume you mean it fails without your patch, but succeeds with it? If so,
> just commit the test case after the patch that fixes it. If not, then we don't
> want this test, because tests should always pass.

I am fine with reordering, I just wanted to exhibit current broken behaviour.

> >(truth be said, a read of 0 isn't necessarily an eof or invalid read
> >for a namedpipe, if the other end of the pipe did a Write(0) it is a
> >normal condition, which isn't possible to distinguish atm with GIO
> >api, since it maps to EOF or BROKEN_PIPE, but that's a different
> >problem anyway)
> 
> this doesn't really belong in the commit message

Ok to remove this blurb if we take the ERROR_MORE_DATA patch

(In reply to comment #42)
> (From update of attachment 220687 [details] [review])
> >+  if (g_cancellable_make_pollfd (cancellable, &pollfd[1]))
> >+    num++;
> 
> ah, that answers my previous question...
> 
> this looks right I guess

would you change patch status to accept_commit or there is still something to fix?

yeah, go ahead and commit the patches, reordering to make the tests always pass though

Attachment 220616 [details] pushed as b9b2cf6 - win32: add pipe-io-concurrent
Attachment 220617 [details] pushed as 23d80a0 - win32: use overlapped events for streams
Attachment 220618 [details] pushed as 4b5d762 - win32: handle ERROR_MORE_DATA
Attachment 220687 [details] pushed as b9d7b80 - win32: make gio stream cancellable
Attachment 220700 [details] pushed as d9f6314 - win32: add pipe-io-cancel-test