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: do not Shutdown during Close #2122

Closed
gopherbot opened this issue Aug 1, 2011 · 11 comments

Comments

Projects
None yet
4 participants
@gopherbot
Copy link

commented Aug 1, 2011

by ziutek@Lnet.pl:

net.TcpConn.File() documentation is wrong or net.TcpConn.Close() have a bug

"File returns a copy of the underlying os.File, set to blocking mode. It is the
caller's responsibility to close f when finished. Closing c does not affect f, and
closing f does not affect c."

But net.TcpConn.Close() uses syscall.Shutdown() without any check of fd.sysref.

Besides, it is not safe to use Posix shutdown() function at all because it 
affects all file descriptors that reference to the connection (they can be created using
Posix dup() function or owned by other process after Posix fork()) in contrast to Posix
close() function which affects only one file descriptor.

What steps will reproduce the problem?

1. Compile and run the followed code:

package main

import (
    "net"
    "log"
    "os"
)

const ADDR = "127.0.0.1:12345"

func checkErr(err os.Error) {
    if err != nil {
        log.Fatalln(err)
    }
}

func connHandler(c net.Conn) {
    buf := make([]byte, 80)
    for {
        n, err := c.Read(buf)
        if err != nil {
            if err == os.EOF {
                c.Close()
            } else {
                log.Println(err)
            }
            return
        }
        os.Stdout.Write(buf[:n])
    }
}

func runServer() {
    l, err := net.Listen("tcp", ADDR)
    checkErr(err)
    go func() {
        for {
            c, err := l.Accept()
            if err != nil {
                log.Println(err)
                continue
            }
            go connHandler(c)
        }
    }()
}

func main() {
    runServer()

    c, err := net.Dial("tcp", ADDR)
    checkErr(err)

    f, err := c.(*net.TCPConn).File()
    checkErr(err)

    checkErr(c.Close())

    _, err = f.Write([]byte("Test\r\n"))
    checkErr(err)

    checkErr(f.Close())
}

What is the expected output?

It should work without any errors.

What do you see instead?

2011/08/01 13:37:22 write tcp:127.0.0.1:33221->127.0.0.1:12345: broken pipe

Which compiler are you using (5g, 6g, 8g, gccgo)?

 8g 

Which operating system are you using?

 Linux

Which revision are you using?  (hg identify)

 43d0a011c089 tip
@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2011

Comment 1:

Status changed to LongTerm.

@alberts

This comment has been minimized.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2011

Comment 3:

It's fine to remove Shutdown as long as you manage to 
(1) keep from closing a file descriptor until there is 
no chance of any goroutines using it in a system call, 
(2) make sure that Close causes any blocked reads 
and writes to wake up and return whatever error it is 
that they return. 
Shutdown was the easiest way to ensure both of those 
but it may be possible to do without it.  If you can, great!

Status changed to HelpWanted.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2011

Comment 5:

Labels changed: added priority-later.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2011

Comment 6:

Labels changed: added priority-go1.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2012

Comment 7:

Owner changed to builder@golang.org.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2012

Comment 8:

I'd love to see this fixed, but I do not see an easy fix that accomplishes what is asked
in comment #3.
It is too close to Go 1 to be rewriting something this fundamental and subtle, so this
will have to wait
until afterward.

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

Owner changed to ---.

@alberts

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2012

Comment 9:

Makes sense. I've looked at this code a few times in an attempt to fix it, but it's not
trivial. It would be really great to get this fixed ASAP. This issue prevents Go from
working nicely with systemd's socket activation.
@gopherbot

This comment has been minimized.

Copy link
Author

commented Feb 11, 2012

Comment 10 by rcostu:

I have been looking at this issue for two hours and I have some notes to do, but I don't
know if I am right on my thinking so correct me if not.
Error on the program shown above fixes changing syscall.Shutdown by a syscall.Close on
the fd.sysfd. As far as I know if you're closing the fd and later checking on references
so as not to have any race condition, should be enough, shouldn't it?
@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2012

Comment 11:

Owner changed to @rsc.

Status changed to Started.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2012

Comment 12:

This issue was closed by revision 5e4e3d8.

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.