golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.6k stars 17.61k forks source link

net: errClosing not exported #4373

Closed gopherbot closed 4 years ago

gopherbot commented 11 years ago

by stephen@q5comm.com:

I have seen the issue in 1.0.3 and from checking tip.golang.org, it is still a problem.

net.errClosing is an errors.New() that is not exported but returned to the user. This
means that in order for a user to check for the "errClosing", they need to
check the error string.

I suggest renaming to ErrClosing so that a client can easily check if another goroutine
closed it's connection.
gopherbot commented 11 years ago

Comment 1 by stephen@q5comm.com:

Another option would be to return a syscall.EINVAL just like reading from a closed
connection. However, that would not be backwards compatible with anyone who is checking
the Error() string and would imply a syscall had taken place and failed.
If backwards compatibility was not an issue, I would probably want to make it so both
reading from a closed connection and a closing connection returned an ErrClosed.
davecheney commented 11 years ago

Comment 2:

Hello,
Can you give a bit of background why you need to know this particular error state ? 
The reason I ask is errClosing is an artefact of net.TCPConn implementations of the
net.Conn interface and is not guaranteed (or ever advertised, hence why it is not
exposed). 
For example, the net.Conn your program may be working with may wrapped by another
implementation, like the ssh package's ssh.ClientConn.
Cheers
Dave

Status changed to WaitingForReply.

alberts commented 11 years ago

Comment 3:

Based on my experience, there seems to be quite a few patterns with goroutines in
servers where you end up with 2 goroutines associated with the same Conn and where
developers (correctly, or not) deem it necessary that both goroutines close that Conn in
some error paths.
In this case, some people want to distinguish between errClosing, which is an expected
error and can be ignored, and any other error from Close (my favourite is EBADF), which
should probably be fatal to the program (i.e. the program should panic when it happens).
The general vibe seems to be that one should simply ignore the error from Close instead
of trying to distinguish between expected and unexpected errors, but this still doesn't
sit completely well with me...
gopherbot commented 11 years ago

Comment 4 by jimenezrick:

As the previous comment clearly states, sometimes a goroutine signals another goroutine
that it should stop reading from that socket. As with channels, closing the resource,
the socket in this case, seems like simple solution.
Cosmetic improvement: if this global value is ever exposed, should it be rename to
"ErrClosed"?
gopherbot commented 11 years ago

Comment 5 by stephen@q5comm.com:

That would imply it is also sent when the net.Conn is closed. Currently it returns the
error of the underlying syscall. If you are not trying to Read/Write at the time the
channel is closed, you would not receive this "net.ErrClosed".
davecheney commented 11 years ago

Comment 6:

A comment on a related issue https://golang.org/issue/4369?c=3
suggested that Accept() pre-empted by a Close should return io.EOF. Would that solution
be acceptable ?
davecheney commented 11 years ago

Comment 7:

For discussion http://golang.org/cl/6852070
rsc commented 11 years ago

Comment 8:

As much as I hate to do it, I think it is probably better to export
ErrClosing. These aren't EOFs.
davecheney commented 11 years ago

Comment 9:

My plan was to reduce the number of paths that could return errClosing with the hope of
eliminating it completely, or returning io.EOF if it was more appropriate.
rsc commented 11 years ago

Comment 10:

By itself that sounds fine, but I really think Closing and EOF are
different things and should be distinguished.
c.Read
c.Close
c.Read // EOF because you are confused and called Close
is different from
c.Read
c.Read // EOF because other side hung up
Russ
davecheney commented 11 years ago

Comment 11:

From the small play I had last week, I think there are problems where errClosing might
be wrapped by another error, from memory this is somewhere in the Accept path.
I agree that ErrClosing is not io.EOF, but I am worried about making this an additional
requirement of all implementations of net.Conn/net.Listener.
rsc commented 11 years ago

Comment 12:

I object to the 'rename errClosing to io.EOF' solution, but I don't
care much what else we do. I suggest either:
1) Define that people should not care, that code that wants to check
for errClosing is buggy to begin with. This is similar to getting rid
of closed(c).
2) Export ErrClosing.
Russ
davecheney commented 11 years ago

Comment 13:

+bradfitz, because of his comment, https://golang.org/issue/4369?c=3
I'd prefer to do 1, relying on errClosing is buggy, for the following:
* most times the errClosing is wrapped in an net.OpError, so a test like err ==
ErrClosing wouldn't work as expected. I'm sure there are situations where the error is
wrapped more than once. I suspect the OP didn't consider that.
* as an implementer of net.Conn implementations in things like the ssh package I
wouldn't like the additional burdon of having to return net.ErrClosing according to an
additional requirement of the net.Conn interface. I'm not even sure that I could detect
concurrent closes on a ssh channel muxed over an unknown net.Conn. Even if the original
ErrClosing was detected and retained, it would be very heavily wrapped, which speaks to
my previous point
* such a change to the net.Conn interface may make existing net.Conn implementations
broken according to the Go 1.x contract.
* Would this change bubble down to io.Reader/Writers ? It is easy to construct a struct
like
c := net.Dial(...)
v := struct { io.Reader; io.Writer; io.Closer }{ c, c, c }
Would there be an implied requirement to return a known ErrClosing when reading from an
io.Reader whose direct implementation was closed concurrently?
Fullung talked about not wanting to ignore errors from Close. I agree with this
sentiment, but wonder if the requirement could be restated as, "I would like to be able
to distinguish between expected and unexpected errors from Close". For the former they
could be safely ignored, the latter might mean a trip through os.Exit(). At the moment
errClosing, being unexported, makes it hard to tell if it is in the expected, or
unexpected class. I believe this is the core issue, and possibly what bradfitz was
suggesting when he suggested replacing errClosing with io.EOF.
At the risk of appearing obstructionist, and considering the number of lines spilt in
this CL vs the size of the original request, I'd like to hear from the OP about their
specific requirements.
bradfitz commented 11 years ago

Comment 14:

I wanted to be able to distinguish a listener closing due to my own closing it versus a
serious problem, but I don't care too much:  If I closed it, I know I closed it, even if
I have to pass that state around or make a net.Listener wrapper that does what I want.
So don't make things complicated for me, if this is hard.
davecheney commented 11 years ago

Comment 15:

@bradfitz, if we're talking about just specifying ErrClosing as a known response from
net.Listener.Accept() then I think that is reasonable. I think specifying it for all
methods on net.Conn is going to let a Genie out that we can't put back.
mikioh commented 11 years ago

Comment 16:

Just an idea.
The net I/O primitives on net.Conn which is created as stream
based bidirectional connection return;
- io.EOF, when the c is closed by the far end,
- io.ErrClosedPipe, when the c is closed by the near end.
rsc commented 11 years ago

Comment 17:

I think we should leave this as is.
Using a locally-Closed net.Conn is an error we won't require a behavior for.

Status changed to WontFix.

davecheney commented 11 years ago

Comment 18:

Issue #5062 has been merged into this issue.

erikdubbelboer commented 9 years ago

This issue was closed by @rsc because of Using a locally-Closed net.Conn is an error we won't require a behavior for.. As shown in #10176 this is actually not always an error and is behavior that is being used by the net/http.Client. I suggest reopening this issue.

ianlancetaylor commented 9 years ago

Reopening for further thought.

mikioh commented 9 years ago

I'm not sure the reason why both net/url and net/http packages don't conform with net.Error interface, I'd prefer to make net.Error interface (both Timeout and Temporary methods on error) work with both packages. Thoughts?

PS: #4856 is already filed for fixing the same issue of net package.

smithwinston commented 9 years ago

I also have two use cases for detecting a closed connection when using goroutines:

  1. Remote end has closed the connection normally
  2. Closing a connection locally in order to stop a service

In both cases, my goroutine is blocked on a Read() and errors out with an *errors.errorString with the text "use of closed network connection".

In both use cases, I want to be able to detect these errors as io.EOF (use case 1) or io.ErrClosed (use case 2) and handle them accordingly. With the current errClosing, I can't cleanly detect the error and if the error message were to change, my code wouldn't detect it.

davecheney commented 9 years ago

In both use cases, I want to be able to detect these errors as io.EOF (use case 1) or io.ErrClosed (use case 2) and handle them accordingly.

Can you please explain how you would handle these cases specifically if you could identify them ?

On Fri, Mar 20, 2015 at 7:18 AM, smithwinston notifications@github.com wrote:

I also have two use cases for detecting a closed connection when using goroutines:

  1. Remote end has closed the connection normally
  2. Closing a connection locally in order to stop a service

In both cases, my goroutine is blocked on a Read() and errors out with an *errors.errorString with the text "use of closed network connection".

In both use cases, I want to be able to detect these errors as io.EOF (use case 1) or io.ErrClosed (use case 2) and handle them accordingly. With the current errClosing, I can't cleanly detect the error and if the error message were to change, my code wouldn't detect it.

— Reply to this email directly or view it on GitHub https://github.com/golang/go/issues/4373#issuecomment-83748662.

smithwinston commented 9 years ago

The general strategy I use is to have a "reader" goroutine sitting in a [blocking] Read() passing any received []byte buffers to channel. I use ragel based parsers to parse the incoming []byte slices into higher order ResponseMessage's. There's also a "writer" that takes a RequestMessage, calls "ToWire()" to get a []byte representation and Write()'s it to the network. Usually an inbound ResponseMessage is matched up to the pending RequestMessage (via a channel), occasionally I have unsolicited messages that are posted to a separate channel for out-of-bound processing.

This describes the steady-state of the system, aside from normal processing of messages, one of three things can happen:

  1. Remote server closes & disconnects (e.g. it might be restarting etc).
  2. Network error (read timeout, host unreachable etc)
  3. Local shutdown (e.g. systemctl stop myservice.service)

1 & 2 are similar, in general the system moves to a mode of attempting to reconnect while pausing operations -- the underlying RequestMessage instances will timeout waiting for responses and the application handle that appropriately. In general, if I'm seeing a connection closed by remote host, then I typically suppress the error. If I'm seeing other kinds of network errors, I want to log them.

As for 3, I close the network connection from main() (in response to a signal) which forces the "reader" out of it's Read() with an error. Again, I don't want to report the error if it's ErrClosed, the reader should shutdown quietly. Otherwise, the reader should be attempting a reconnection.

Typically, the "reader" handles any reconnecting as the "writer" is usually blocking on a select waiting for messages to send, but it's possible that the writer will encounter the error before the reader does. In either case, I call Close() on the connection to ensure that both the reader and writer both see the error.

Linux: dead connection detection I have some additional logic in the writer to check the kernel output queue size by calling ioctl(fd, SIOCOUTQ) after a select/time.After(30s) on the write channel. If there's still data in the queue, then it's likely the connection has failed somewhere along the line (despite using TCP_KEEPALIVE) and I need to bounce the connection by calling Close() on it.

davecheney commented 9 years ago

So the cause of the error has no effect on the logic of your program, the cause only effects the decision to mute reporting of the error in a log? Did I understand that correctly?

On 20 Mar 2015, at 08:01, davem-intersys notifications@github.com wrote:

The general strategy I use is to have a "reader" goroutine sitting in a [blocking] Read() passing any received []byte buffers to channel. I use ragel based parsers to parse the incoming []byte slices into higher order ResponseMessage's. There's also a "writer" that takes a RequestMessage, calls "ToWire()" to get a []byte representation and Write()'s it to the network. Usually an inbound ResponseMessage is matched up to the pending RequestMessage (via a channel), occasionally I have unsolicited messages that are posted to a separate channel for out-of-bound processing.

This describes the steady-state of the system, aside from normal processing of messages, one of three things can happen:

Remote server closes & disconnects (e.g. it might be restarting etc). Network error (read timeout, host unreachable etc) Local shutdown (e.g. systemctl stop myservice.service) 1 & 2 are similar, in general the system moves to a mode of attempting to reconnect while pausing operations -- the underlying RequestMessage instances will timeout waiting for responses and the application handle that appropriately. In general, if I'm seeing a connection closed by remote host, then I typically suppress the error. If I'm seeing other kinds of network errors, I want to log them.

As for 3, I close the network connection from main() (in response to a signal) which forces the "reader" out of it's Read() with an error. Again, I don't want to report the error if it's ErrClosed, the reader should shutdown quietly. Otherwise, the reader should be attempting a reconnection.

Typically, the "reader" handles any reconnecting as the "writer" is usually blocking on a select waiting for messages to send, but it's possible that the writer will encounter the error before the reader does. In either case, I call Close() on the connection to ensure that both the reader and writer both see the error.

Linux: dead connection detection I have some additional logic in the writer to check the kernel output queue size by calling ioctl(fd, SIOCOUTQ) after a select/time.After(30s) on the write channel. If there's still data in the queue, then it's likely the connection has failed somewhere along the line (despite using TCP_KEEPALIVE) and I need to bounce the connection by calling Close() on it.

— Reply to this email directly or view it on GitHub.

smithwinston commented 9 years ago

Yes, any error would be logged as a warning, ErrClosed would not be logged. Getting ErrClosed causes the readers/writers to exit (this could be in response to a shutdown, or alternately, it could be to restart a connection).

davecheney commented 9 years ago

Thanks for clarifying.

The error philosophy for Go is that errors should be opaque to the caller and from what I am hearing you can already do this.

Are there cases currently that you consider an error cause to be temporary that you are not able to identify? From the description you have provided whenever an error is returned the handling code must close and reopen the socket so it sounds like no change is necessary.

The case of closing the socket concurrently in the shutdown situation should probably be handled with a flag on the retry loop of your program, not by pushing more logic down into the net package.

Yes, any error would be logged as a warning, ErrClosed would not be logged. Getting ErrClosed causes the readers/writers to exit (this could be in response to a shutdown, or alternately, it could be to restart a connection).

— Reply to this email directly or view it on GitHub https://github.com/golang/go/issues/4373#issuecomment-83776675.

smithwinston commented 9 years ago

The error philosophy for Go is that errors should be opaque to the caller and from what I am hearing you can already do this.

It just seems that the errClosing is an oddity; Close()ing the io.ReadWriteCloser seems to be a common use case as it's the only way to "break" a goroutine out of a blocking Read().

BTW: I'm going from this blog posting as "best practices" for error handling. I typically define my own errors as follows:

var ErrMessageResponseTimeout = errors.New(“Message Response Timeout”)

So I can take action on particular errors.

Are there cases currently that you consider an error cause to be temporary that you are not able to identify?

I don't think so, I don't know what a temporary network error would even be? (I assume the runtime handles things like EBUSY or EINTR).

The case of closing the socket concurrently in the shutdown situation should probably be handled with a flag on the retry loop of your program,

Yes, in m design that's how I do it; in response to the error, the reader/writers exit. If a shutdown is in progress, this has already been signaled and the retry loop ends. If there's no shutdown flag, the retry loop will create a new connection and restart the reader/writers.

I guess you can argue that there's always a case when an ErrClosing is intentional and thus should be indicated by some kind of shutdown flag. But it sure would be nice to be able not to clutter the error log on errClosing without having to do a string match!

davecheney commented 9 years ago

What if instead of returning errClosing we fixed the concurrent close behaviour to return io.EOF ? On 20 Mar 2015 09:34, "smithwinston" notifications@github.com wrote:

The error philosophy for Go is that errors should be opaque to the caller and from what I am hearing you can already do this.

It just seems that the errClosing is an oddity; Close()ing the io.ReadWriteCloser seems to be a common use case as it's the only way to "break" a goroutine out of a blocking Read().

BTW: I'm going from this blog posting http://blog.golang.org/error-handling-and-go as "best practices" for error handling. I typically define my own errors as follows:

var ErrMessageResponseTimeout = errors.New(“Message Response Timeout”)

So I can take action on particular errors.

Are there cases currently that you consider an error cause to be temporary that you are not able to identify?

I don't think so, I don't know what a temporary network error would even be? (I assume the runtime handles things like EBUSY or EINTR).

The case of closing the socket concurrently in the shutdown situation should probably be handled with a flag on the retry loop of your program,

Yes, in m design that's how I do it; in response to the error, the reader/writers exit. If a shutdown is in progress, this has already been signaled and the retry loop ends. If there's no shutdown flag, the retry loop will create a new connection and restart the reader/writers.

I guess you can argue that there's always a case when an ErrClosing is intentional and thus should be indicated by some kind of shutdown flag. But it sure would be nice to be able not to clutter the error log on errClosing without having to do a string match!

— Reply to this email directly or view it on GitHub https://github.com/golang/go/issues/4373#issuecomment-83787777.

smithwinston commented 9 years ago

What if instead of returning errClosing we fixed the concurrent close behaviour to return io.EOF ?

That would be OK by me, but I thought I had read elsewhere on one of these issues that people wanted to know the difference between local-Close() [ErrClosing] and remote disconnection [io.EOF].

davecheney commented 9 years ago

That would be OK by me, but I thought I had read elsewhere on one of these issues that people wanted to know the difference between local-Close() [ErrClosing] and remote disconnection [io.EOF].

Based on the use case provided above the requirement to distinguish between local closed (ie, this error is expected because I asked for it in another goroutine) and unexpected remote disconnection (hey, where did you go ?) does not change the logic of the program; any non nil error is considered non temporary and will cause the same logic to be executed.

What remains from this request is to be able to mute errors of the former variety from being logged. By removing errClosing and always returning io.EOF, this requirement is met.

On Fri, Mar 20, 2015 at 9:48 AM, smithwinston notifications@github.com wrote:

What if instead of returning errClosing we fixed the concurrent close behaviour to return io.EOF ?

That would be OK by me, but I thought I had read elsewhere on one of these issues that people wanted to know the difference between local-Close() [ErrClosing] and remote disconnection [io.EOF].

— Reply to this email directly or view it on GitHub https://github.com/golang/go/issues/4373#issuecomment-83789631.

smithwinston commented 9 years ago

By removing errClosing and always returning io.EOF, this requirement is met.

Agreed.

davecheney commented 9 years ago

@ianlancetaylor @ErikDubbelboer @mikioh if I were to prototype a change which removed the errClosing from the Read and Write paths in the net package and replaced it with io.EOF would this be an acceptable solution ?

note: I don't know what bug this will uncover or trigger, but in general is this a workable solution ?

erikdubbelboer commented 9 years ago

@davecheney It would be preferable over detecting an error by checking the error string.

ianlancetaylor commented 9 years ago

@davecheney I have to say that Russ's comments above seem right to me: errClosing is not EOF. I think that returning io.EOF for errClosing will simplify some current code but at the cost of future confusion by programmers trying to understand why they are getting EOF.

I think the error happens when two different goroutines have a net.Conn, and one goroutine calls Close, and the other goroutine calls Read (or friends) or Write (or friends) or Close or CloseRead or CloseWrite. Or, similarly, with a TCPListener or UnixListener where one goroutine calls Close and the other calls Accept. At least for Read/Write/Accept this is clearly an error condition but I don't see why it is an end-of-file. What happens do a regular socket descriptor if you close and then read? You don't get EOF. You get EINVAL.

In a comment above you said the errClosing might be wrapped in a net.OpError, but I don't see how that can happen. Can you explain the case where that can occur?

I think it might be useful to distinguish two cases. One is where the read/write is in progress, and then some other goroutine closes the connection. I think we have to return errClosing for that case. Nothing else properly conveys what happened. The other is where the close happens, and then after that some other goroutine does a read/write. For that I think we could return a different error (errClosed?). Perhaps if we distinguish those cases we can separate ourselves from concerns that all other interfaces need to support errClosing, and then perhaps we can export it.

davecheney commented 9 years ago

@ianlancetaylor thank you for your comprehensive reply. In response I have re-read the discussion in this issue and need to clarify my statements.

  1. Initially this issue was around net.Listener.Accept() returning an exported type so that callers could distinguish between concurrently closing their own listener, and some other error from the net.Listener implementation.

I believe through the discussion this use case was debated and found to be insufficient justification.

  1. In Nov 2012 or there abouts I proposed replacing errClosing with io.EOF and this was rejected. In the three years that followed, I forgot this, sorry.

In this current discussion it appears to me that the request is for implementations of net.Conn.Read and net.Conn.Write, which currently return errClosing return an exported type. The motivation is similar, callers want to be able to identify their own actions which resulted in a spurious and this ignorable error. I believe while the motivation is the same as the original request, the specifics are different and this may allow a different conclusion than was reached in 2012.

In a comment above you said the errClosing might be wrapped in a net.OpError, but I don't see how that can happen. Can you explain the case where that can occur?

So much has changed in the net package since I made those comments I cannot give a compelling justification for those statements, I withdraw that part of my objection.

I think it might be useful to distinguish two cases. One is where the read/write is in progress, and then some other goroutine closes the connection. I think we have to return errClosing for that case. Nothing else properly conveys what happened.

I agree with what you have said, but I don't believe that this issue is about should we return io.EOF vs net.errClosing, what the OP wants is that errClosing be exported so they can check it and quash logging of the error.

The OP has indicated that the cause of the error, io.EOF, net.errClosing, or some other non nil error does not affect their program's logic, only the tertiary process of logging the error.

Furthermore the OP only appears to be concerned with errors coming from Read and Write; Accept and Close can be considered out of scope for this discussion -- or at least I consider them out of scope.

To reduce the scope even more, focussing on the net.Conn.Read method I think that it is reasonable to return io.EOF when the socket is closed as there is no more data to read. To me io.EOF means just that, there is no more data that can be read -- it is mute on why that is the case.

Perhaps if we distinguish those cases we can separate ourselves from concerns that all other interfaces need to support errClosing, and then perhaps we can export it.

At the moment we're discussing the types in the net package only, as they are the only ones with access to net.errClosed, exporting net.ErrClosed so that it was part of the contract of io.Read/Write/Closers that were embedded in the net.Conn interface is something I would argue against because it would break the Go 1 guarantee.

ianlancetaylor commented 9 years ago

My concern with returning io.EOF is that I think we're just being pushed there by history, and I don't find that sufficiently compelling in this case. I think if we were to look with fresh eyes and say "what should happen if I close a net.Conn and then call the Read method" I don't think we would reach for io.EOF.

ianlancetaylor commented 9 years ago

I don't quite follow the argument about exporting ErrClosed (did you mean ErrClosing) and the Go 1 guarantee. Can you expand on that? Thanks.

davecheney commented 9 years ago

I don't quite follow the argument about exporting ErrClosed (did you mean ErrClosing) and the Go 1 guarantee. Can you expand on that? Thanks.

I am sorry, I transposed one for the other. My argument can be better summarised as the net.Conn interface inherits its Read and Write methods from the corresponding interfaces in the io package. Those interfaces only specify the behaviour of one error value, io.EOF.

If an exported type, call it net.ErrClosed, the name is unimportant, is specified for net.Conn.Read interface method then any implementation of a net.Conn is bound by this new behaviour. For example, tls.Conn will have to be checked to ensure that it properly propagates errors matching net.ErrClosed up to the caller who is relying on this concurrent close contract. This also applies to every other type that satisfies net.Conn. To not return net.ErrClosing in situations when the underlying net.Conn, possibly itself wrapped several times, does, would make that implementation a violation of the net.Conn interface contract.

I see this as an additional requirement over and above the Go 1 promise, and a disproportionate cost to pay given the justifications provided by the OP.

davecheney commented 9 years ago

My concern with returning io.EOF is that I think we're just being pushed there by history, and I don't find that sufficiently compelling in this case. I think if we were to look with fresh eyes and say "what should happen if I close a net.Conn and then call the Read method" I don't think we would reach for io.EOF.

I agree, my suggestion of io.EOF is a compromise that will satisfy the request that self inflicted errors can be quashed. It is an abuse of io.EOF's position in the io.Reader contract but I think justified for the case of net.Conn.Read.

mikioh commented 9 years ago

I'm not keen on changing the existing error notification style; io.EOF and other.

  1. io.EOF indicates no data to read on the endpoint that is created as connection-oriented protocol
  2. In other cases, net.Error indicates whether the endpoint is still usable

I still don't see the reason why the net package should expose some conditional value for error notification instead of fixing net/url, net/http packages.

davecheney commented 9 years ago

Mikio,

Could you give more details here. I remember there was an issue with an error value being masked deep down in the resolution of a URL, but I think that is a different issue to the one being discussed here.

On 22 Mar 2015, at 11:53, Mikio Hara notifications@github.com wrote:

I'm not keen on changing the existing error notification style; io.EOF and other.

io.EOF indicates no data to read on the endpoint that is created as connection-oriented protocol In other cases, net.Error indicates whether the endpoint is still usable I still don't see the reason why the net package should expose some conditional value for error notification instead of fixing net/url, net/http packages.

— Reply to this email directly or view it on GitHub.

mikioh commented 9 years ago

I think that is a different issue to the one being discussed here.

I don't know, to me, it seems that the OP just wants to know whether the endpoint is usable for his/her internal cleanups/tasks. In addition, why not io.ErrClosedPipe?

davecheney commented 9 years ago

I don't know, to me, it seems that the OP just want to know whether the endpoint is usable for his/her internal cleanups/tasks. In addition, why not ErrClosedPipe?

I don't see this as a discussion about the name of the exported value. The OP asked us to export net.errClosing and through this discussion it's been discovered that exporting this would be used to help suppress expected errors during async close of a reading socket.

My concern is that this would also create a new contract on the net.Conn.Read method, that all implementations must return this new exported value when closed concurrently, and they must also respect and bubble it up from types that they wrap. Also, while the OP has indicated that for their particular example this new exported type would not be used to affect program flow, I am certain that users will quickly come to depend on this value.

This is a slippery slope which I do not want to embark on. So, as a counter suggestion I am proposing replacing returning net.errClosing with io.EOF in as limited a scope as possible to satisfy the requirements of the OP without introducing a new contract for all net.Conn.Read implementations.

I recognise that this is not a perfect solution, but I feel I have given strong arguments why adding extra exported values to the net.Conn contract is a bad idea.

mikioh commented 9 years ago

We introduced io.EOF into the net package (a disadvantage: it prevents to convey additional network information with net.OpError in the case of returning io.EOF), and we agreed with it because we thought that the result of any read operation on connection-oriented protocol: nread==0 && err==nil is equivalent to io.EOF.

But in this case, I'm not sure your proposal is the way to go. Here are my random thoughts.

  1. At the moment, we provide io.EOF and net.Error compliant values for part of error notification protocol/contact on IO operations of connection-oriented protocol endpoints.
  2. If we need to introduce a new signal for error notification, it should follow io package because net.Conn implements io.Reader/Writer interfaces and it would be a rationale for introducing a disadvantage the same as io.EOF.
  3. net.OpError or other net.Error compliant types/values are important for making diagnostics after disaster happened; I mean, for management plane stuff. IMHO, for control plane stuff, determining the failure from error notifications come from service access points/APIs of network protocols doesn't matter because SAPs/APIs usually don't satisfy such soft/hard error determination. (anyone shows me the perfect compliant of RFC 5461?)
davecheney commented 9 years ago

We know you introduced io.EOF to the net package (a disadvantage: it prevents to convey additional network information with net.OpError in the case of returning io.EOF), and we agreed with it because we thought that the result of any read operation on connection-oriented protocol: nread==0 && err==nil is equivalent to io.EOF.

Mate, that was a bit uncalled for. I'm not sure what set of people you classify as 'we' that doesn't include me, but I think it is unfair to blame me for the fact that net.Conn.Read obeys the io.Reader contract.

On Sun, Mar 22, 2015 at 2:30 PM, Mikio Hara notifications@github.com wrote:

We know you introduced io.EOF to the net package (a disadvantage: it prevents to convey additional network information with net.OpError in the case of returning io.EOF), and we agreed with it because we thought that the result of any read operation on connection-oriented protocol: nread==0 && err==nil is equivalent to io.EOF.

But in this case, I'm not sure your proposal is the way to go. Here are my random thoughts.

  1. At the moment, we provide io.EOF and net.Error compliant values for part of error notification protocol/contact on IO operations of connection-oriented protocol endpoints.
  2. If we need to introduce a new signal for error notification, it should follow io package because net.Conn implements io.Reader/Writer interfaces and it would be a rationale for introducing a disadvantage the same as io.EOF.
  3. net.OpError or other net.Error compliant types/values are important for making diagnostics after disaster happened; I mean, for management plane stuff. IMHO, for control plane stuff, determining the failure from error notifications come from service access points/APIs of network protocols don't matter because SAPs/APIs usually don't satisfy such soft/hard error determination. (anyone shows me the perfect compliant of RFC 5461?)

— Reply to this email directly or view it on GitHub https://github.com/golang/go/issues/4373#issuecomment-84510711.

mikioh commented 9 years ago

No, I'm not intended to blame you, and you are right, (edited my previous comment); sorry about that.

davecheney commented 9 years ago

Ok, thanks for clarifying.

rsc commented 9 years ago

Putting on the maybe list, but it's not exported because people shouldn't use it.

erikdubbelboer commented 9 years ago

@rsc I would prefer not using it as well but since it's the only way to detect a http.Client timeout I have to.

People are now using their own solutions like: https://github.com/facebookgo/grace/blob/master/grace.go#L44