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: fix Dialer, DialTimeout regression #11796

Closed
dsymonds opened this issue Jul 20, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@dsymonds
Copy link
Member

commented Jul 20, 2015

Since 0d8366e, net.DialTimeout appears to no longer respect timeouts shorter than 2s. That is, if I do a net.DialTimeout("tcp", "some-addr", 500*time.Millisecond) where "some-addr" is something that resolves quickly (with both IPv4 and IPv6 addresses) but doesn't respond to connections, net.DialTimeout takes a little over 1s to return.

This has changed since Go 1.4.2. I don't know whether this was an intentional change, or a regression; the former change warrants a mention in the release notes.

@pmarks-net @bradfitz @mikioh

@dsymonds dsymonds added this to the Go1.5 milestone Jul 20, 2015

dsymonds added a commit to golang/appengine that referenced this issue Jul 20, 2015

Work around suspect upstream bug in Go for API dial timeout test.
golang/go#11796 has details of the bug.

Change-Id: I94aa78685c962c32652b227b1597076c305977ac
@pmarks-net

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2015

Could you clarify what problem you're seeing? The 2s saneMinimum only affects how time is distributed across multiple addresses; it should not change the overall deadline. The following example shows a short timeout working as expected:

$ cat ~/test/dial_fail.go
package main

import (
        "net"
        "fmt"
        "time"
)

func main() {
        startTime := time.Now()
        conn, err := net.DialTimeout("tcp", "192.0.2.1:80", 123*time.Millisecond)
        fmt.Printf("%v %v\n", conn, err)
        fmt.Printf("Finished in %v\n", time.Now().Sub(startTime))
}

$ ../bin/go run ~/test/dial_fail.go
<nil> dial tcp 192.0.2.1:80: i/o timeout
Finished in 123.238884ms
@dsymonds

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2015

@pmarks-net

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2015

Ah, I am able to reproduce a bug in the partialDeadline calculation. When dialing 2 addresses with a 5 second timeout, the target deadline is recomputed for each address, instead of once at the beginning of Dial:

$ ../bin/go run ~/test/dial_fail.go 
partialDeadline=2.499999662s
partialDeadline=4.999999028s
<nil> dial tcp [2001:db8::1]:80: i/o timeout
Finished in 7.500665372s

I introduced the bug between patch sets 3-4 of the code review:
https://go-review.googlesource.com/#/c/8768/3..4/src/net/dial.go

@pmarks-net

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2015

There is actually a second, pre-existing bug: DialTimeout(..., 5s) will allocate 5 seconds to DNS resolution, and another 5 seconds to TCP.

The fix for both is to call d.deadline(...) exactly once, at the beginning of Dial.

@bradfitz bradfitz changed the title Document or fix net.DialTimeout regression net: document or fix DialTimeout regression Jul 20, 2015

@gopherbot

This comment has been minimized.

Copy link

commented Jul 21, 2015

CL https://golang.org/cl/12442 mentions this issue.

@dsymonds dsymonds added the Started label Jul 21, 2015

@bradfitz bradfitz closed this in dcc905e Jul 23, 2015

@dsymonds dsymonds removed the Started label Jul 23, 2015

@mikioh mikioh changed the title net: document or fix DialTimeout regression net: fix Dialer, DialTimeout regression Jul 23, 2015

@golang golang locked and limited conversation to collaborators Aug 5, 2016

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.