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

os: document that NewFile can return nil for invalid fd #20023

Closed
bontibon opened this issue Apr 18, 2017 · 7 comments

Comments

@bontibon
Copy link
Contributor

commented Apr 18, 2017

Since the os.NewFile function does not return an error value, one might assume that the function always returns a non-nil value. A line in the docs saying otherwise might save some users a little grief.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2017

I don't think this is a documentation bug; NewFile can only return nil if someone passes a negative value. Given we cannot change the signature now, I argue this method should panic, as the construction of an invalid value of fd is a programming error.

@ALTree

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

panic or nil, the doc should say "panics / returns nil when..". The fact that panicing is better than returning nil is a differe issue IMHO.

@ALTree ALTree added this to the Go1.9 milestone Apr 20, 2017
@ALTree

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

Noting in the doc that NewFile can return nil "when the file descriptor uint is not valid" is band-aid; dave wrote on CL 41211:

I don't think this is an improvement. What is an invalid file descriptor? Is it a number larger than ulimit -n? Is it something that was obtained from os.File.Fd() and since closed? Is it a negative fd number? technically yes, but given the function accepts unsigned values.

I think that rather than getting stuck in a documentation rabbit hole, we should do either:

  1. panic, rather than return nil
  2. or, return some *os.File that is already closed.

Moving this to Proposal as suggested to decide if a (vague) doc addition is enough or we also should change the function to panic instead.

@ALTree ALTree modified the milestones: Proposal, Go1.9 Apr 21, 2017
@ALTree ALTree changed the title os: docs do not state that NewFile can return nil proposal: make os.NewFile panic on bad file descriptor Apr 21, 2017
@gopherbot

This comment has been minimized.

Copy link

commented Apr 21, 2017

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

@rsc rsc changed the title proposal: make os.NewFile panic on bad file descriptor proposal: os: make NewFile panic on bad file descriptor May 15, 2017
@rsc

This comment has been minimized.

Copy link
Contributor

commented May 15, 2017

It has always behaved this way (return nil for fd < 0, back to 2008), but it does seem weird that fd=-1 is handled differently from fd=1e9. Perhaps we should return a non-nil *File that returns an appropriate error from future operations (whatever happens when you use the fd later).

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 15, 2017

Actually, all the methods already return ErrInvalid for nil *File and the docs for ErrInvalid already say "methods on File will return this error when the receiver is nil". So maybe this is fine and just a doc update is needed.

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 15, 2017

Based on discussion with @golang/proposal-review (reflected above), the doc change seems to be the best, least disruptive path forward. Since nil is usable and has been forever, it seems OK to continue that and just document how it happens.

@rsc rsc changed the title proposal: os: make NewFile panic on bad file descriptor proposal: os: document that NewFile can return nil for invalid fd May 15, 2017
@rsc rsc changed the title proposal: os: document that NewFile can return nil for invalid fd os: document that NewFile can return nil for invalid fd May 15, 2017
@rsc rsc modified the milestones: Go1.9, Proposal May 15, 2017
@gopherbot gopherbot closed this in 9a43255 May 18, 2017
@golang golang locked and limited conversation to collaborators May 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.