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: ECONNABORTED = death #3395

Closed
dhobsd opened this issue Mar 26, 2012 · 8 comments

Comments

Projects
None yet
4 participants
@dhobsd
Copy link
Contributor

commented Mar 26, 2012

It's possible for a connection to arrive but close while it's on the listen queue. When
you call accept(2) on this connection, it will return ECONNABORTED. I can't think of any
reasons for net/http (or any other network listener APIs we have) to care about this.
Maybe they should be logged, but they certainly shouldn't cause me to reset all the
connections in the listen queue (and in the case that people exit after an error in the
listener, all in-flight connections).

Anyway, I'd like some additional input before writing a patch for this. The error isn't
"Temporary" or "Timeout" -- it's ... Ignorable. Kind of like
SIGPIPE, which we ignore in pkg/runtime. I thought of adding something like:

func (e Errno) Ignorable() bool {
  return e == ECONNABORTED
}

into syscall_unix.go and syscall_windows.go (it means the same thing in both), and then
adding an "ignorable" interface in net.go with OpError() implementing that
interface and calling into the syscall level, just like Temporary and Timeout do. I
don't know that this is what we want to do architecturally, though. I feel like
"Ignorable" is the wrong word. Should we just check ne.Err ==
syscall.ECONNABORTED after we call l.Accept() everywhere? That seems to make less sense.

Also, I don't think it makes sense to do everywhere. There are probably applications
where you're talking over a local TCP socket and this would constitute an error
condition -- so I don't think it's something that we should test for in AcceptTCP. I
think though generally where this can happen in crypto/tls, net/http, net/http/fcgi, and
net/rpc it's probably not a real error.

I'm happy to make a patch for this issue, and I think it's probably something that
should go in Go1. If it doesn't, it would probably be pretty easy to DoS basically any
Go server on a public address. I'd expect that people writing their own services would
determine whether this is an issue for themselves, but I really can't think of any
reason we shouldn't effectively ignore this error in all of our existing TCP-based APIs.
Thoughts?
@dsymonds

This comment has been minimized.

Copy link
Member

commented Mar 26, 2012

Comment 1:

It seems that a closed socket should never be returned by os.Accept (it's pretty
pointless). Just my two cents.

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

@rsc

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2012

Comment 2:

I'm happy for the net package to treat ECONNABORTED as 'go around
again' during the select loop for Accept.
That's a pretty silly error.
@dhobsd

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2012

Comment 3:

Thanks, patch incoming.
@rsc

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2012

Comment 4:

Let's delay this until after Go 1, however.
@dhobsd

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2012

Comment 5:

Ok, though I don't quite understand why. All I have to do is try to connect and RST the
socket in a tight loop and basically any Go server will die at some point, FreeBSD,
Linux or Windows. Anyhow, 5905063 created for whenever it's ready.
@rsc

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2012

Comment 6:

Go 1 is going to ship with bugs.  That is unavoidable.
The question is whether it ships with bugs we know about
or bugs we don't know about.  Making last-minute changes
is a great way to create new bugs we don't know about.
Russ
@dhobsd

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2012

Comment 7:

Fair enough :)
@rsc

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2012

Comment 8:

This issue was closed by revision a63c37b.

Status changed to Fixed.

@dhobsd dhobsd added fixed labels Mar 27, 2012

@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.