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: errClosing not exported #4373

Open
gopherbot opened this Issue Nov 11, 2012 · 87 comments

Comments

Projects
None yet
@gopherbot

gopherbot commented Nov 11, 2012

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

This comment has been minimized.

gopherbot commented Nov 11, 2012

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

This comment has been minimized.

Contributor

davecheney commented Nov 14, 2012

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

This comment has been minimized.

Contributor

alberts commented Nov 14, 2012

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

This comment has been minimized.

gopherbot commented Nov 18, 2012

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

This comment has been minimized.

gopherbot commented Nov 18, 2012

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

This comment has been minimized.

Contributor

davecheney commented Nov 19, 2012

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

This comment has been minimized.

Contributor

davecheney commented Nov 20, 2012

Comment 7:

For discussion http://golang.org/cl/6852070
@rsc

This comment has been minimized.

Contributor

rsc commented Nov 26, 2012

Comment 8:

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

This comment has been minimized.

Contributor

davecheney commented Nov 26, 2012

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

This comment has been minimized.

Contributor

rsc commented Nov 26, 2012

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

This comment has been minimized.

Contributor

davecheney commented Nov 27, 2012

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

This comment has been minimized.

Contributor

rsc commented Nov 27, 2012

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

This comment has been minimized.

Contributor

davecheney commented Nov 27, 2012

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

This comment has been minimized.

Member

bradfitz commented Nov 28, 2012

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

This comment has been minimized.

Contributor

davecheney commented Nov 28, 2012

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

This comment has been minimized.

Contributor

mikioh commented Nov 28, 2012

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

This comment has been minimized.

Contributor

rsc commented Dec 9, 2012

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

This comment has been minimized.

Contributor

davecheney commented Mar 16, 2013

Comment 18:

Issue #5062 has been merged into this issue.

@erikdubbelboer

This comment has been minimized.

Contributor

erikdubbelboer commented Mar 17, 2015

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

This comment has been minimized.

Contributor

ianlancetaylor commented Mar 17, 2015

Reopening for further thought.

@mikioh

This comment has been minimized.

Contributor

mikioh commented Mar 17, 2015

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

This comment has been minimized.

smithwinston commented Mar 19, 2015

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

This comment has been minimized.

Contributor

davecheney commented Mar 19, 2015

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
#4373 (comment).

gopherbot pushed a commit that referenced this issue Apr 26, 2017

os, net, internal/poll: return consistent error for closed socket
In the past we returned "use of closed network connection" when using
a closed network descriptor in some way. In CL 36799 that was changed
to return "use of closed file or network connection". Because programs
have no access to a value of this error type (see issue #4373) they
resort to doing direct string comparisons (see issue #19252). This CL
restores the old error string so that we don't break programs
unnecessarily with the 1.9 release.

This adds a test to the net package for the expected string.

For symmetry check that the os package returns the expected error,
which for os already exists as os.ErrClosed.

Updates #4373.
Fixed #19252.

Change-Id: I5b83fd12cfa03501a077cad9336499b819f4a38b
Reviewed-on: https://go-review.googlesource.com/39997
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@mdlayher

This comment has been minimized.

Member

mdlayher commented Nov 28, 2017

Hi all, I've frequently run into this problem, and have noticed it even affects the stdlib (albeit via x/net/http2 bundle): https://github.com/golang/net/blob/master/http2/server.go#L625-L631.

I know a func IsClosed(err error) bool probably isn't ideal, but it would be excellent to have some way to detect a closed network connection other than comparing against an unexported string value that can change between releases.

@awishformore

This comment has been minimized.

awishformore commented Dec 19, 2017

I can't believe I just read through all of this thread and there is no solution. This situation even creates friction in the standard library, as the poster before me has pointed out. How is this not an issue? I believe this comment really said it all.

Being able to code around something does not mean the straight forward way is not a better solution. I don't believe there is a valid reason not to expose ErrClosed. Just like the standard library, I will also resort to detecting the error by string. I don't want to do it, but considering the other options, it's still the best approach to keep my code clean.

@awishformore

This comment has been minimized.

awishformore commented Dec 19, 2017

To elaborate a bit on my view: Go advertises use of channels heavily and I use them when running many concurrent i/o operations. I use flow-based or reactor-inspired architectures a lot. The io.Reader and io.Writer interfaces always create friction because calls on them lock. I already have to use workarounds such as casting to a TCP reader/writer/listener and applying deadlines to cleanly integrate goroutines working on network connections. The only way to keep this kind of architecture consistent is by checking the error string; I'm not going to change the whole application architecture, which is elegant and works great, to code around this issue.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Dec 19, 2017

@awishformore I'm sympathetic, but there are specific objections above to exporting ErrClosing. It would be helpful if you could address those directly. Thanks.

@awishformore

This comment has been minimized.

awishformore commented Dec 19, 2017

@ianlancetaylor I would prefer if this would not turn into a discussion about making error handling consistent across the standard library, because that seems to be what is holding back the change. The comment I linked summarizes the correct response to the objections.

This is Go, let's be pragmatic. Is there a clear use case for exposing the error? Yes. Would it keep the code cleaner and easier to understand? Yes. Are there io.Reader/io.Writer implementations that return their own sentinel errors? Yes.

We have a simple solution to a problem that is clearly relevant. Why do we still have to have code in the standard library that detects a specific kind of error by comparing strings?

Personally, I am not a big fan of sentinel errors and I much prefer the interface approach used by the net package, but the interfaces used are what they are and already return sentinel errors. It would break no contract at all to simply expose an already existing error, and it would be the pragmatic thing to do.

To me, the slippery slope argument is fallacious in this case. This is a very specific case with a very specific purpose. Nobody will start suggesting these kind of changes everywhere if you expose this error.

@powerman

This comment has been minimized.

powerman commented Dec 20, 2017

Isn't the os.IsExist-family of functions does exactly the same thing - unpack underlying error and compare it to some internal value (in os case it's OS-specific errors, but to me this doesn't differs too much from unexported internal/poll.ErrNetClosing)? Maybe the best we can do is just add similar net.IsClosed(err error) bool?

If I understand @mdlayher correctly he proposed same thing in #4373 (comment) few weeks ago and it's already most ++ed comment in this 5-years-old thread… sounds like consensus?

And yes, detecting this error is mostly about correct logging case, but, hey, logging is important, even if it doesn't affect flow control.

As for writing own wrapper proposed by @bradfitz, all examples of such wrappers in this thread are missing mutex around modifying bool variable. Personally I really prefer net.IsClosed() over wrapper with mutex.

BTW, there is one more use case not mentioned in this thread - concurrent calls to conn.Close() from different goroutines. This may happens, for example, if buffered channel used to send []byte to "writer" goroutine is full (because of slow reader client) and several goroutines was trying to send something to this channel in non-blocking way, failed because channel is full, and called conn.Close() to disconnect that slow reader at same time. In this case one conn.Close() will return no error while other return ErrNetClosing - and shouldn't write it to log.

@awishformore

This comment has been minimized.

awishformore commented Dec 20, 2017

For reference, I have code similar to this in three separate projects now:

func isClosedErr(err error) bool {
	if err == nil {
		return false
	}
	err = errors.Cause(err)
	if strings.Contains(err.Error(), "use of closed network connection") {
		return true
	}
	return false
}

I even put it into a separate file called workaround.go for the last one.

@hubslave

This comment has been minimized.

hubslave commented Mar 25, 2018

I have code similar to the following in a few of my projects too.
In fact, if you ever need to close a Listener, I don't know how you could avoid it.
That is not so often but it happens (for example you usually want to close an unix domain socket).
There is a similar example in another comment but it's incomplete.

type wrapper struct {
     net.Listener
     closed bool
     mu     sync.Mutex
}

func (w *wrapper) Close() error {
     w.mu.Lock()
     defer w.mu.Unlock()
     err := w.Listener.Close()
     w.closed = w.closed || err == nil
     return err
}

func (w *wrapper) IsClosed() bool {
     w.mu.Lock()
     defer w.mu.Unlock()
     return w.closed
}
w := &wrapper{Listener: ln} // wrapping
// ...
conn, err := w.Accept()
if err != nil {
    if w.IsClosed() {
        // handle self inflicted error
    } else {
        // handle error
    }
}

Acceptable solution, but I would welcome any cut down on the boilerplate.
Maybe it would be possible to make an IsClosed method available through a type assertion without changing the Listener interface.
I also like the func IsSelfInflicted(err error) bool idea.

@pascaldekloe

This comment has been minimized.

Contributor

pascaldekloe commented Mar 26, 2018

@hubslave the error could have been be raised before close (false positives). Also no need for mutex on Boolean type.

@hubslave

This comment has been minimized.

hubslave commented Mar 26, 2018

@pascaldekloe

the error could have been be raised before close (false positives).

Not perfect, I'm aware of that.
I don't even know what kind of non-self inflicted errors Accept can return, but to ignore one that happened just before a successful shut down of the listener seems of little consequences.
bradfitz suggested the same workaround to someone with a similar problem, and I still like it better then the parsing of the error string, which I understand it's the only other option.

Also no need for mutex on Boolean type.

Can you give me a reference for that?
From The Go Memory Model I would understand the opposite.

@tv42

This comment has been minimized.

tv42 commented Mar 29, 2018

@pascaldekloe

Also no need for mutex on Boolean type.

That's a race. It could be an atomic instead of a mutex-protected bool, though, but that's splitting hairs.

@hubslave A very typical scenario would be "listen until this Context cancels", and Context gives you the equivalent of this IsClosed for free.

herenow pushed a commit to herenow/jrpc2 that referenced this issue Jun 25, 2018

Handle reads from closed sockets and pipes better.
After closing a pipe or socket, a read will return an internal error whose text
contains "use of closed network connection". Unfortunately, that appears to be
the only way to detect that error, and golang/go#4373
suggests it's not going to improve on its own.

Do what ~everyone else seems to have done in this case, check for the string.
That gives us nicer diagnostics when a client or server shuts down in the
normal course of its connections being terminated.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 27, 2018

@networkimprov

This comment has been minimized.

networkimprov commented Aug 11, 2018

Because apps now rely on the value of net.Error.Error() to detect errClosing, that string will never change in Go 1.x.

Just codify that string in the docs and declare victory until Go2 :-)

@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 14, 2018

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 14, 2018

In Go 1.13, we're going to try to push out the error chaining changes. At that point, finding a good home for errClosing might be worth doing.

@networkimprov

This comment has been minimized.

networkimprov commented Nov 16, 2018

@rsc by "error chaining" are you referring to the Go2 Error Values stdlib proposal or Go2 Error Handling language concept?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Nov 16, 2018

@networkimprov The Error Values proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment