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: Dial returns non-nil connection on error #6614

Closed
calmh opened this issue Oct 18, 2013 · 11 comments

Comments

Projects
None yet
7 participants
@calmh
Copy link
Contributor

commented Oct 18, 2013

In go1.1 net.Dial() used to return a nil connection (interface) on errors; now it
returns a non-nil interface with a nil value. I don't know if this is intentional,
unintentional but not a problem or an issue -- you judge.

This is with go1.2rc2 / tip, Mac OS X.

The following test case succeeds in go1.1 and fails with go1.2rc2 (assuming you don't
have anything listening on 127.0.0.1:65535):

http://play.golang.org/p/GQiQkCQUIQ

//jb
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Comment 1:

I believe this is working as intended for two reasons:
1. your sample code is faulty, if there is no error, then you program will likely crash,
http://play.golang.org/p/z7T_IVGDr2
2. the net.Dial() function makes no guarantees about the contents of Conn, or the error
implementations that may be returned. The only contract is the general contract of
checking the error value before making any assumption about the validity of the other
return values.
@calmh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2013

Comment 2:

Dave,
Dude... :) That's not sample code, that's a testcase with a very specific condition
("assuming you don't have anything listening on 127.0.0.1:65535") to illustrate what I'm
talking about only. And like I said, not sure if it was an issue or not, but it was a
change I noticed and it's generally considered cleaner to return a nil interface than a
non-nil interface with a nil value. Feel free to close with works-as-intended if that's
the case, that's fine by me.
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Comment 3:

Mikio, I'm guessing this is part of the changes around the time we switched to the
runtime poller for the *BSDs. 
I'm sticking to my line that there is no guarantee about the value of Conn if the error
isn't handled first, but the part of about returning a typed nil makes my old war wound
ache. 
What are you thought on this ?

Owner changed to @mikioh.

@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Comment 4:

I don't think it matters. If you get a non-nil error and use the Conn anyway, you
deserve whatever you get.

Status changed to WorkingAsIntended.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Comment 5:

Jakob,
> it's generally considered cleaner to return a nil interface than a non-nil interface
with a nil value.
Thanks for the notice. Maybe we'll have a chance to revisit this, such beauty in Go 2
but I personally want to fix the issue "net package is weird because it returns weird
errors" first. So I'm gonna close this issue for now.
Dave, 
> we switched to the runtime poller for the *BSDs.
Actually no, in a series of Happy Eyeballs CLs. See changeset: 17772:3833ddddde2b. FWIW,
net.Listen already behaves so since Go 1.0. Also both net.Dial and net.Listen still
return a nil interface on error when you give invalid arguments.
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Comment 6:

@mikio thanks for correcting me.  Although I do agree strongly with comment #4, typed
nils are such a time bomb I do wonder if it is worth adding a test case to assure they
do not leak out of the net package (that is assuming they started leaking out after 1.1)
@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 18, 2013

Comment 7:

I would also like to see this fixed.  It can't be hard.
@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Comment 8:

It should return a nil net.Conn.

Labels changed: added go1.2.

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Comment 9:

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

Owner changed to @rsc.

Status changed to Started.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Comment 11:

This issue was closed by revision 66f49f7.

Status changed to Fixed.

@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2013

Comment 12:

This issue was closed by revision d2862cc2bfc5.

@calmh calmh added fixed labels Oct 31, 2013

@rsc rsc added this to the Go1.2 milestone Apr 14, 2015

@rsc rsc removed the go1.2 label Apr 14, 2015

adg added a commit that referenced this issue May 11, 2015

[release-branch.go1.2] net: make sure failed Dial returns nil Conn
««« CL 14950045 / 1e60ffd5933d
net: make sure failed Dial returns nil Conn

Fixes #6614.

R=golang-dev, bradfitz, mikioh.mikioh
CC=golang-dev
https://golang.org/cl/14950045
»»»

R=golang-dev
CC=golang-dev
https://golang.org/cl/20170047

@golang golang locked and limited conversation to collaborators Jun 25, 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.