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

@alexbrainman
Copy link
Member

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

rsc commented Oct 6, 2012

Comment 1:

Labels changed: added go1.1.

@alexbrainman
Copy link
Member Author

Comment 2:

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

@alexbrainman
Copy link
Member Author

@davecheney
Copy link
Contributor

Comment 4:

Alex, is this issue resolved ?

@alexbrainman
Copy link
Member Author

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

rsc commented Dec 10, 2012

Comment 6:

Labels changed: added size-l.

@rsc
Copy link
Contributor

rsc commented Dec 30, 2012

Comment 7:

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

@rsc
Copy link
Contributor

rsc commented Mar 12, 2013

Comment 8:

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

@robpike
Copy link
Contributor

robpike commented May 18, 2013

Comment 9:

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

@robpike
Copy link
Contributor

robpike commented Aug 19, 2013

Comment 10:

Status changed to Fixed.

@dvyukov
Copy link
Member

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

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

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

dvyukov commented Aug 22, 2013

Comment 14:

Status changed to Accepted.

@alexbrainman
Copy link
Member Author

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

dvyukov commented Aug 23, 2013

Comment 16:

see comment #11

@alexbrainman
Copy link
Member Author

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

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

Comment 19:

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

@alexbrainman
Copy link
Member Author

Comment 20:

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

Status changed to Started.

@alexbrainman
Copy link
Member Author

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.
Projects
None yet
Development

No branches or pull requests

6 participants