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

Jump to bottom

os: document file descriptor ownership semantics for os.NewFile() #43863

Closed
vikmik opened this issue Jan 23, 2021 · 2 comments
Closed

os: document file descriptor ownership semantics for os.NewFile() #43863

vikmik opened this issue Jan 23, 2021 · 2 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@vikmik
Copy link
Contributor

vikmik commented Jan 23, 2021 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.15.6 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/vic/.cache/go-build"
GOENV="/home/vic/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/vic/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/vic/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.15"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.15/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/vic/go/src/github.com/optimyze/prodfiler/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build483022862=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I am not providing a repro case here, as this is dependent on GC pressure & is intended behavior
What I did: I used os.NewFile with a file descriptor owned by a different piece of code. To illustrate:

func func1(fd uintptr) {
    myFile := os.NewFile(fd, "")
    [do some stuff with myFile, like using interfaces that require an io.ReadSeeker]
    [not closing myFile, not closing fd]
}

func func2() {
    fd := getFDFromSomewhere()
    func1(fd)
    // code using fd after this is unedefined behavior
    // Indeed, fd might get closed at any point when `myFile` (in `func1`) gets garbage collected
}

This seems to be intended behavior because of: https://github.com/golang/go/blob/master/src/os/file_unix.go#L178
The GC behavior is documented on os.File.Fd(): https://github.com/golang/go/blob/master/src/os/file_unix.go#L65

What did you expect to see?

I expected to see a comment on os.NewFile warning me that the naked file descriptor can only be used while the returned os.File isn't garbage-collected

What did you see instead?

Documentation isn't explicit (unless I was looking at the wrong place?), os.NewFIle doesn't mention garbage collection at all.
This triggered a difficult-to-reproduce/diagnose bug in my service - I had to go and read the Golang source code to find the root cause.

I'll be happy to open a PR to fix the documentation if this issue seems legit.
Thanks!
@vikmik vikmik changed the title os: document os.File ownership semantics for os.NewFile() os: document file descriptor ownership semantics for os.NewFile() Jan 23, 2021
vikmik added a commit to vikmik/go that referenced this issue Jan 23, 2021
@vikmik
os: document file descriptor ownership for NewFile
f60863a 
NewFile requires the file descriptor to be either closed
through the returned File instance, or to stay valid at least
until the finalizer runs during garbage collection.

These requirements are easily violated when file descriptors
are closed via unix.Close, or when the File returned by
NewFile is garbage collected while the file descriptor is still
valid and in use.

This commit adds further documentation for NewFile, making it
explicit that the file descriptor must only be closed via the
returned File.

Fixes golang#43863
@vikmik vikmik mentioned this issue Jan 23, 2021
Closed
@vikmik
Copy link
Contributor Author

vikmik commented Jan 23, 2021 •
edited

Not sure if documentation PRs are looked at without discussion here - sorry if this isn't the case. I posted #43867 / https://golang.org/cl/286032

@gopherbot
Copy link

Change https://golang.org/cl/286032 mentions this issue: os: document file descriptor ownership for NewFile

vikmik added a commit to vikmik/go that referenced this issue Jan 24, 2021
@vikmik
os: document file descriptor ownership for NewFile
f157c9a 
NewFile requires the file descriptor to be either closed
through the returned File instance, or to stay valid at least
until the finalizer runs during garbage collection.

These requirements are easily violated when file descriptors
are closed via unix.Close, or when the File returned by
NewFile is garbage collected while the file descriptor is still
valid and in use.

This commit adds further documentation for NewFile, making it
explicit that the file descriptor must only be closed via the
returned File.

Fixes golang#43863
vikmik added a commit to vikmik/go that referenced this issue Jan 24, 2021
@vikmik
os: further document limitations around naked file descriptors
180d013 
NewFile requires the file descriptor to be either closed
through the returned File instance, or to stay valid at least
until the finalizer runs during garbage collection.

These requirements are easily violated when file descriptors
are closed via unix.Close, or when the *File returned by
NewFile is garbage collected while the underlying file descriptor is
still in use.

This commit adds further documentation for NewFile and Fd, making it
explicit that using naked file descriptors is subject to constraints
due to garbage collection of File objects.

Fixes golang#43863
@toothrot toothrot added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 25, 2021
@toothrot toothrot added this to the Backlog milestone Jan 25, 2021
@golang golang locked and limited conversation to collaborators Jan 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

os: further document limitations around naked file descriptors vikmik/go
3 participants
@toothrot @vikmik @gopherbot