Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
crvv opened this issue Oct 30, 2018 · 12 comments
Closed

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

crvv opened this issue Oct 30, 2018 · 12 comments

Comments

@crvv
Copy link
Contributor

@crvv crvv commented Oct 30, 2018

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

  1. On Windows, (*os.File).Close() cancels pending I/O operations for all files. I think this is a good feature.
  2. #25835 is fixed. The other ways to fix this are much more complex.
  3. #23019 can be fixed on Windows. #23019 has the same issue with #25835.

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

@gopherbot gopherbot added this to the Proposal milestone Oct 30, 2018
@gopherbot gopherbot added the Proposal label Oct 30, 2018
@ianlancetaylor 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 ianlancetaylor modified the milestones: Proposal, Unplanned Oct 30, 2018
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 30, 2018

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

CC @alexbrainman

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Oct 31, 2018

@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
Copy link
Contributor Author

@crvv crvv commented Oct 31, 2018

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 31, 2018

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
Copy link
Contributor Author

@crvv crvv commented Oct 31, 2018

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
Copy link
Member

@alexbrainman alexbrainman commented Nov 11, 2018

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
Copy link

@astrieanna astrieanna commented Jan 15, 2019

@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
Copy link
Member

@alexbrainman alexbrainman commented Jan 17, 2019

@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
Copy link

@astrieanna astrieanna commented Jan 17, 2019

@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
Copy link
Contributor Author

@crvv crvv commented Jan 18, 2019

@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
Copy link
Member

@alexbrainman alexbrainman commented Jan 18, 2019

@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

crvv added a commit to crvv/go that referenced this issue Mar 4, 2019
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
crvv added a commit to crvv/go that referenced this issue Mar 4, 2019
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
Copy link

@gopherbot gopherbot commented Mar 4, 2019

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

@gopherbot gopherbot closed this in d227a08 Mar 19, 2019
@golang golang locked and limited conversation to collaborators Mar 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.