internal/poll: cancel pending I/O in FD.Close() on Windows #28477

crvv · 2018-10-30T05:09:14Z

I propose this, roughly

 // Close closes the FD. The underlying file descriptor is closed by
 // the destroy method when there are no remaining references.
 func (fd *FD) Close() error {
        if !fd.fdmu.increfAndClose() {
                return errClosing(fd.isFile)
        }
+       if !fd.pollable {
+               syscall.CancelIoEx(fd.Sysfd, nil)
+       }
        // unblock pending reader and writer
        fd.pd.evict()
        err := fd.decref()
        // Wait until the descriptor is closed. If this was the only
        // reference, it is already closed.
        runtime_Semacquire(&fd.csema)
        return err
 }

And the result is

On Windows, (*os.File).Close() cancels pending I/O operations for all files. I think this is a good feature.
os/exec: Wait behavior changed from Go 1.9 to Go 1.10 on Windows #25835 is fixed. The other ways to fix this are much more complex.
os/exec: consider changing Wait to stop copying goroutines rather than waiting for them #23019 can be fixed on Windows. os/exec: consider changing Wait to stop copying goroutines rather than waiting for them #23019 has the same issue with os/exec: Wait behavior changed from Go 1.9 to Go 1.10 on Windows #25835.

Other notes:
CancelIoEx requires Windows vista or later.
https://docs.microsoft.com/en-us/windows/desktop/fileio/cancelioex-func

The text was updated successfully, but these errors were encountered:

ianlancetaylor · 2018-10-30T14:50:04Z

This sounds like a bug fix, not a proposal, so I've adjusted the labels.

CC @alexbrainman

alexbrainman · 2018-10-31T09:12:43Z

@crvv if it is a bug fix, I need to see description of a bug. Otherwise I do not know what your change fixes.

Please answer these questions so I can understand what your bug is. Thank you.

What version of Go are you using (`go version`)?

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (`go env`)?

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

What did you expect to see?

What did you see instead?

crvv · 2018-10-31T09:36:57Z

I think I have wrote what the change fix.

#25835 is fixed.

I don't think this issue is a bug fix. It is not a bug for Close not cancelling pending I/O.
This issue just proposes a behavior change and the change happens to fix a bug.

ianlancetaylor · 2018-10-31T14:09:46Z

I don't understand the issue here. Someone is going to have to send a change that includes a test case. @crvv: Do you want to do that? Thanks.

crvv · 2018-10-31T15:39:55Z

I think I should add more details to this issue.

It is about this behavior of Close
https://golang.org/pkg/os/#File.Close

On files that support SetDeadline, any pending I/O operations will be canceled and return immediately with an error.

This is the current situation. The issue is going to cancel pending I/O operations for all files on Windows.

Why this change can fix #25835?

#25835 is that Close is blocked by a concurrent Read. It is equivalent to

What operating system and processor architecture are you using (go env)?

Windows, AMD64

What did you do?

https://play.golang.org/p/Avsxc4QYDhd

What did you expect to see?

call r.Read()
call r.Close()
closed
read |0: file already closed

What did you see instead?

call r.Read()
call r.Close()

and hangs forever.

#25835 is just a complex example of this.
On Unix, the bug is fixed by 0f3ab14

alexbrainman · 2018-11-11T05:54:39Z

On Unix, the bug is fixed by 0f3ab14

The change adds TestCloseWithBlockingRead to the os package. But TestCloseWithBlockingRead (along with all others in pipe_test.go source file) does not run on Windows. I think, first step here - we should make TestCloseWithBlockingRead and all other tests in pipe_test.go run on Windows. And then we will decide what to do next. I will try to do that when I have time.

Thank you.

Alex

astrieanna · 2019-01-15T20:00:20Z

@alexbrainman I think I'm running into this bug when trying to run this Kubernetes test program on Windows.

In this specific test it runs this program, connects via TCP, then disconnects. When run on Windows, the program gets stuck in the Read call forever. On Linux, it exits when the connection is closed.

This behavior is also exercised by the TestCloseWithBlockingReadByFd that you mentioned earlier and TestFdReadRace in the same file. TestCloseWithBlockingReadByFd fails on windows due to getting stuck in Read and Close. TestFdReadRace hangs forever (unless go test is invoked with -timeout).
Calling syscall.CancelIoEx(fd.Sysfd, nil) in the Close function, as suggested here, does cause the Read to return (with an error, not io.EOF).

alexbrainman · 2019-01-17T08:30:32Z

@astrieanna thank you for your comment here.

Calling syscall.CancelIoEx(fd.Sysfd, nil) in the Close function, as suggested here, does cause the Read to return (with an error, not io.EOF).

I did not have a chance to investigate what the problem is yet. So I would not know, if proposed solution is good. Like I said earlier, I will try to debug this when I have time.

If you want to fix this yourself, feel free to do so. Here https://golang.org/doc/contribute.html is how to contribute to Go project.

Alex

astrieanna · 2019-01-17T14:32:59Z

@alexbrainman I'd be happy to fix this. My main concern is that I'm not sure when not to run the CancelIoEx function. The suggestion from this issue is to check a pollable property that doesn't currently exist, but I'm not sure what that is intended to do. With your greater familiarity with this code base, could you give any guidance on what would break if CancelIoEx were run on every close to help me come up with the right conditional?

crvv · 2019-01-18T04:14:30Z

@astrieanna
The package internal/poll is used by os and net.
File descriptors in net package is in non-blocking mode (pollable).
In os package, they are in blocking mode(not pollable).
(There is an issue for using non-blocking IO in os package on Windows, #19098)

I guess there are not side effects if cancelIoEx is called in every close.
But if the fd is in non-blocking mode, cancelIoEx is useless.

alexbrainman · 2019-01-18T07:59:45Z

@alexbrainman I'd be happy to fix this. My main concern is that I'm not sure when not to run the CancelIoEx function. The suggestion from this issue is to check a pollable property that doesn't currently exist, but I'm not sure what that is intended to do. With your greater familiarity with this code base, could you give any guidance on what would break if CancelIoEx were run on every close to help me come up with the right conditional?

This issue only affects pipes. I would try and avoid changing code that affects other file types. Maybe you could use GetFileType Windows API to determine if file is pipe.

CancelIoEx sounds OK to me, if it fixes the tests.

I think the biggest problem here is to make all tests in pipe_test.go (nearly all) pass before we commit to a solution.

Alex

When closing a pipe, use CancelIoEx to cancel pending I/O. This makes concurrent Read and Write calls to return os.ErrClosed. This change also enables pipe_test on Windows. Fixes golang#28477, golang#25835 Change-Id: If52bb7d80895763488a61632e4682a78336e8420

When closing a pipe, use CancelIoEx to cancel pending I/O. This makes concurrent Read and Write calls to return os.ErrClosed. This change also enables some pipe tests on Windows. Fixes golang#28477, golang#25835 Change-Id: If52bb7d80895763488a61632e4682a78336e8420

gopherbot · 2019-03-04T12:06:24Z

Change https://golang.org/cl/164721 mentions this issue: internal/poll, os: cancel pending I/O when closing pipes on Windows

gopherbot added this to the Proposal milestone Oct 30, 2018

gopherbot added the Proposal label Oct 30, 2018

ianlancetaylor changed the title ~~proposal: internal/poll: cancel pending I/O in FD.Close() on Windows~~ internal/poll: cancel pending I/O in FD.Close() on Windows Oct 30, 2018

ianlancetaylor added NeedsInvestigation and removed Proposal labels Oct 30, 2018

ianlancetaylor modified the milestones: Proposal, Unplanned Oct 30, 2018

ianlancetaylor added the OS-Windows label Oct 30, 2018

alexbrainman mentioned this issue Dec 21, 2018

os: consider syscall.EEXIST in os.IsExist #29295

Open

gopherbot closed this as completed in d227a08 Mar 19, 2019

benmoss mentioned this issue Jul 22, 2019

windows: kubectl port-forward does not work kubernetes/kubernetes#78857

Closed

golang locked and limited conversation to collaborators Mar 18, 2020

gopherbot added the FrozenDueToAge label Mar 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/poll: cancel pending I/O in FD.Close() on Windows #28477

internal/poll: cancel pending I/O in FD.Close() on Windows #28477

crvv commented Oct 30, 2018

ianlancetaylor commented Oct 30, 2018

alexbrainman commented Oct 31, 2018

crvv commented Oct 31, 2018

ianlancetaylor commented Oct 31, 2018

crvv commented Oct 31, 2018

alexbrainman commented Nov 11, 2018

astrieanna commented Jan 15, 2019

alexbrainman commented Jan 17, 2019

astrieanna commented Jan 17, 2019

crvv commented Jan 18, 2019

alexbrainman commented Jan 18, 2019

gopherbot commented Mar 4, 2019

internal/poll: cancel pending I/O in FD.Close() on Windows #28477

internal/poll: cancel pending I/O in FD.Close() on Windows #28477

Comments

crvv commented Oct 30, 2018

ianlancetaylor commented Oct 30, 2018

alexbrainman commented Oct 31, 2018

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

What did you do?

What did you expect to see?

What did you see instead?

crvv commented Oct 31, 2018

ianlancetaylor commented Oct 31, 2018

crvv commented Oct 31, 2018

Why this change can fix #25835?

What operating system and processor architecture are you using (go env)?

What did you do?

What did you expect to see?

What did you see instead?

alexbrainman commented Nov 11, 2018

astrieanna commented Jan 15, 2019

alexbrainman commented Jan 17, 2019

astrieanna commented Jan 17, 2019

crvv commented Jan 18, 2019

alexbrainman commented Jan 18, 2019

gopherbot commented Mar 4, 2019

What version of Go are you using (`go version`)?

What operating system and processor architecture are you using (`go env`)?