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

net: setting read deadline affects write deadline, and vice verse (on windows) #4195

Closed
alexbrainman opened this issue Oct 5, 2012 · 21 comments

Comments

Projects
None yet
6 participants
@alexbrainman
Copy link
Member

commented Oct 5, 2012

What steps will reproduce the problem?

run this on windows:

package main

import (
    "log"
    "net"
    "time"
)

const (
    readTimeout  = time.Second
    writeTimeout = 2 * time.Second
    delta        = 300 * time.Millisecond
)

func checkTimeout(command string, start time.Time, timeoutShould time.Duration) {
    timeoutIs := time.Now().Sub(start)
    d := timeoutShould - timeoutIs
    if d < -delta || delta < d {
        log.Fatalf("%s TIMEOUT TEST FAILED: is=%v should=%v\n", command, timeoutIs, timeoutShould)
    }
}

func main() {
    ln, err := net.Listen("tcp", "127.0.0.1:0")
    if err != nil {
        log.Fatalf("ListenTCP on :0: %v", err)
    }

    go func() {
        c, err := ln.Accept()
        if err != nil {
            log.Fatalf("Accept: %v", err)
        }
        defer c.Close()

        select {}
    }()

    c, err := net.Dial("tcp", ln.Addr().String())
    if err != nil {
        log.Fatalf("Dial: %v", err)
    }
    defer c.Close()

    start := time.Now()

    quit := make(chan bool)

    go func() {
        err := c.SetReadDeadline(start.Add(readTimeout))
        if err != nil {
            log.Fatalf("SetReadDeadline: %v", err)
        }

        var buf [10]byte
        _, err = c.Read(buf[:])
        if err == nil {
            log.Fatalf("Read should not succeed")
        }

        checkTimeout("READ", start, readTimeout)
        quit <- true
    }()

    go func() {
        err := c.SetWriteDeadline(start.Add(writeTimeout))
        if err != nil {
            log.Fatalf("SetWriteDeadline: %v", err)
        }

        var buf [10000]byte
        for {
            _, err = c.Write(buf[:])
            if err != nil {
                break
            }
        }

        checkTimeout("WRITE", start, writeTimeout)
        quit <- true
    }()

    <-quit
    <-quit
}

What is the expected output?

<nothing>

What do you see instead?

2012/10/05 12:46:01 WRITE TIMEOUT TEST FAILED: is=1.0000128s should=2s


Please use labels and text to provide additional information.

Our current windows implementation uses syscall.CancelIo windows api to stop waiting io.
But this function cancels *ANY* pending io on that socket, not just particular read or
write. I see no proper solution to that problem in general. But on later versions of
Windows we could use new syscall.CancelIoEx api, that can selectively cancel particular
io.

Is it good idia to introduce new complexity (different execution path for different
versions of os) to solve that problem?

CancelIoEx will also allow us not to switch threads during io with deadline (we have
dedicated goroutine that is locked to a single thread that starts every io, because
CancelIo only affects io "started on the same thread"). Also, considering
there will be more and more "new" versions of Windows, it will become more
useful with time.

Alex
@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2012

Comment 1:

Labels changed: added go1.1.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2012

Comment 2:

Some systems will be fixed by http://golang.org/cl/6604072.
Alex
@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2012

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2012

Comment 4:

Alex, is this issue resolved ?
@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2012

Comment 5:

This issue is resolved for system that have CancelIoEx API (as per http://goo.gl/Kuwlz
"... Windows Vista and up ...").
TestReadWriteDeadline that tests the issue is disabled for other systems.
I have no proposal to fix the issue completely.
Alex
@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2012

Comment 6:

Labels changed: added size-l.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2012

Comment 7:

Labels changed: added priority-later, removed priority-triage.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2013

Comment 8:

Labels changed: added go1.1maybe, removed go1.1.

@robpike

This comment has been minimized.

Copy link
Contributor

commented May 18, 2013

Comment 9:

Labels changed: added go1.2maybe, removed go1.1maybe.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2013

Comment 10:

Status changed to Fixed.

@dvyukov

This comment has been minimized.

Copy link
Member

commented Aug 19, 2013

Comment 11:

I am not sure this is fixed.
I think it can be as simple as -- have 2 dedicated threads: 1 for read operations, 1 for
write operations, send IO requests and cancellations to the necessary thread.

Status changed to Accepted.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2013

Comment 12:

Yes, this is still broken, but only on systems that do not have CancelIOEx API. There we
have no choice but to use CancelIO that cancels ALL IO. I don't know any better way of
dealing with it. But, given that we have less and less systems like that, I cannot see
this as been important. So feel free to do what you like with this issue.
Alex
@dvyukov

This comment has been minimized.

Copy link
Member

commented Aug 22, 2013

Comment 13:

OK, let's just remove it from release line (Go1.2) and make Priority-Someday
It seems to be relatively easy to fix, but hard to test, and only affects old OSes, and
only affects "non-typical" network programs that use different deadlines for reads and
writes.
If somebody wants to fix this, you are welcome.

Labels changed: added priority-someday, removed priority-later, go1.2maybe.

Status changed to WontFix.

@dvyukov

This comment has been minimized.

Copy link
Member

commented Aug 22, 2013

Comment 14:

Status changed to Accepted.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2013

Comment 15:

>> It seems to be relatively easy to fix, ...
Tell me.
>> If somebody wants to fix this, you are welcome.
I will try if you have a plan.
Alex
@dvyukov

This comment has been minimized.

Copy link
Member

commented Aug 23, 2013

Comment 16:

see comment #11
@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2013

Comment 17:

I don't see how 2 threads setup is any better then what we have now. We still have only
CancelIo API that cancels *everything*.
Alex
@dvyukov

This comment has been minimized.

Copy link
Member

commented Aug 23, 2013

Comment 18:

CancelIo
"Cancels all pending input and output (I/O) operations that are issued by the calling
thread for the specified file. The function does not cancel I/O operations that other
threads issue for a file handle".
If you have a thread that issues all read operations, and another thread that issues all
write operations, then you can cancel read and write operations separately with
CancelIo. That's exactly what we need. We have at most 1 pending read and at most 1
pending write, so we do not need finer granularity of cancel.
@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2013

Comment 19:

Yes. I forgotten about that. I will see if it is simple enough to implement.
Alex
@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2013

Comment 20:

Please, review https://golang.org/cl/12960046/.
Alex

Status changed to Started.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2013

Comment 21:

This issue was closed by revision 11320fa.

Status changed to Fixed.

@golang golang locked and limited conversation to collaborators Jun 24, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.