code review 6869054: net: document that File reverts connection to blocking mode.

net: document that File reverts connection to blocking mode.

Fixes issue 2458.

[rick, 2012-12-04 01:54:48]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[mikio, 2012-12-04 10:33:53]

Please change the CL description to "net: document File..." or an appropriate
one.

[mikio, 2012-12-04 10:34:09]

https://codereview.appspot.com/6869054/diff/5001/src/pkg/net/net.go
File src/pkg/net/net.go (right):

https://codereview.appspot.com/6869054/diff/5001/src/pkg/net/net.go#newcode204
src/pkg/net/net.go:204: // be used to modify socket options. Doing so may lead
to unexpected errors.
Perhaps it might be better to clarify that each fd owns its descriptor 
flags, but status flags which include O_NONBLOCK will be shared.
In short, File breaks Conn's internal non-blocking I/O and timeouts.

[rick, 2012-12-04 13:45:59]

PTAL

https://codereview.appspot.com/6869054/diff/5001/src/pkg/net/net.go
File src/pkg/net/net.go (right):

https://codereview.appspot.com/6869054/diff/5001/src/pkg/net/net.go#newcode204
src/pkg/net/net.go:204: // be used to modify socket options. Doing so may lead
to unexpected errors.
On 2012/12/04 10:34:09, mikio wrote:
> Perhaps it might be better to clarify that each fd owns its descriptor 
> flags, but status flags which include O_NONBLOCK will be shared.
> In short, File breaks Conn's internal non-blocking I/O and timeouts.

Good idea. Done.

[rsc, 2012-12-04 15:22:09]

https://codereview.appspot.com/6869054/diff/1003/src/pkg/net/net.go
File src/pkg/net/net.go (right):

https://codereview.appspot.com/6869054/diff/1003/src/pkg/net/net.go#newcode203
src/pkg/net/net.go:203: // The file descriptor associated with the returned
os.File shares status
This is too detailed. It's describing a specific implementation, not an API.

// The returned os.File's file descriptor is different from the connection's.
// Attempting to change properties of the original using this duplicate
// may or may not have the desired effect.

might be enough.

[rick, 2012-12-04 17:20:51]

On 2012/12/04 15:22:09, rsc wrote:
> This is too detailed. It's describing a specific implementation, not an API.

I think mikio was trying to get me to document the side effect that calling File
will set both the connection and the returned os.File to blocking mode even if
you don't do anything with the fd directly (since they share the same underlying
status attributes). Should that be documented still?

Thanks.

[mikio, 2012-12-04 22:03:08]

On 2012/12/04 17:20:51, rick wrote:

> I think mikio was trying to get me to document the side effect that calling
File
> will set both the connection and the returned os.File to blocking mode even if
> you don't do anything with the fd directly (since they share the same
underlying
> status attributes). Should that be documented still?

I'm okay with Russ's proposal because it includes the words "using this dup",
"conn's properties" and "undesired effect". Thanks.

[rick, 2012-12-04 23:01:06]

PTAL

How about this simple change to cover both cases?

[mikio, 2012-12-05 05:17:28]

LGTM, leave for others to submit.

[rsc, 2012-12-06 04:31:48]

*** Submitted as https://code.google.com/p/go/source/detail?r=f4d54bcd5313 ***

net: document that File reverts connection to blocking mode.

Fixes issue 2458.

R=mikioh.mikioh, rsc
CC=golang-dev
https://codereview.appspot.com/6869054

Committer: Russ Cox <rsc@golang.org>