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

net: errClosing not exported #4373

Closed
gopherbot opened this issue Nov 11, 2012 · 116 comments
Closed

net: errClosing not exported #4373

gopherbot opened this issue Nov 11, 2012 · 116 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@gopherbot
Copy link

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

Comment 7:

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

@rsc
Copy link
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
Copy link
Contributor

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
Copy link
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
Copy link
Contributor

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
Copy link
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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
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
Copy link
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
Copy link
Contributor

Comment 18:

Issue #5062 has been merged into this issue.

@erikdubbelboer
Copy link
Contributor

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
Copy link
Contributor

Reopening for further thought.

@mikioh
Copy link
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
Copy link

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
Copy link
Contributor

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).

@Miosss
Copy link

Miosss commented Jun 4, 2020

This should be exported (in some way).
I stumbled upon this today, when debugging net.TCPLIstener.accept() return values.

My code correctly closes the listener, and goroutine that accepts clients gets only ""use of closed network connection" as an error, when I would like to differentiate this 'not really an error but perfectly fine way to top this goroutine' from an actual error.

Some may (and have already) ask why it is important to be able to check different errors. I thought it was obvious. The most basic example is not to log the information about normal close, only about true errors. Second is trying to restart the function. If listener truly breaks, then I will try to start it again. If it was closed normally (bacause of context.cancel for example) then I won't.

I understand that go is fun language and doing hard things is easy with it. But the simple ones are usually the most challenging. Especially reliable error handling.

Reluctance to exporting specific errors is something I do not understand. Especially when there are no error codes, nothing more then plain string error in the error interface. Somebody in this issue wrote about http2 abstracting errors. I had the same situation and it mostly boils down to abstracting (or just hiding/obscuring) the true errors. I filled an issue back then at #28930

If this problem is not solved, and only strings will be THE error values, then parsing those string will become idiomatic.
And is is even already recognized in internal/poll/fd.go:

// ErrNetClosing is returned when a network descriptor is used after
// it has been closed. Keep this string consistent because of issue
// #4373: since historically programs have not been able to detect
// this error, they look for the string.
var ErrNetClosing = errors.New("use of closed network connection")

@noncombatant
Copy link

I agree with @riobard in #4373 (comment): it's not necessarily the case that errClosing must be propagated to all other interfaces that wrap or work like this one.

In any case, if a public function returns an object, that object's type must also be public — otherwise the caller has no chance to fully know and understand the interface of the function they're calling. The inevitable result is this string matching silliness (which I also found myself having to do), when type switching is the idiomatic and much more clearly correct approach.

If we're willing to live with designs we don't love, and we have to be ;) , it seems straightforward that exporting the type is much less unlovable than the status quo (#4373 (comment)).

@awfm9
Copy link

awfm9 commented Jun 17, 2020

There are currently over 3,000 cases of matching this error string on GitHub, including projects such as Kubernetes:

https://github.com/search?l=Go&q=%22use+of+closed+network+connection%22&type=Code

Whatever people think is the "correct" way to work around the issue, it is clearly out of touch with the reality on the ground.

Let's please implement the pragmatic solution asap.

@frickenate
Copy link

frickenate commented Aug 10, 2020

Since it's been almost 3 months since the last serious comment here, I'm going to bump this.

I have multiple variations of this across multiple code bases at the moment:

// shortened example; full version has defer Close() and error handling on Listen and Close
server, _ := net.Listen("tcp4", ":9000")

go func() {
    for {
        client, err := server.Accept()
        if err != nil {
            // Accept() returns an error with substring "use of closed network connection" if
            // the socket has been closed elsewhere (ie. during graceful stop, instead of EOF).
            // Go is "supporting" this string comparison until an alternative is presented to
            // improve the situation. See https://github.com/golang/go/issues/4373 for info.
            if !strings.Contains(err.Error(), "use of closed network connection") {
                log.Printf("[fatal] server: socket %q: accept: %v\n", server.Addr(), err)
                cExitCode <- 1
            }
            return
        }

        go handleClient(client)
    }
}()

Go has quite a few "wtf" moments to adjust to, but this has to be one of the worst. Checking for a substring in an error message is just… disgusting. At least, unlike most code repositories, I've commented mine with the "wtf" thought process and a link to this issue so future developers who have to maintain my code might hopefully understand. So many other of the 3000 instances reported by @awfm9 give no such warning to the future.

If it's really necessary to break compatibility, I think we're finally hitting the point where a 2.0 is required. A lot of the core packages are so out of date with "core values". For example, the net package is severely low-level and does not adopt any of the usual Go patterns. Why does net.Timeout() exist, which still requires a typecheck of net.Error or *net.OpError after a Set(Read|Write)Deadline() and Read()? Where is the use of a context, or a wrapped error that lets us use errors.Is() and errors.As()?

At some point, the Go team needs to completely abandon all new features, and focus on a purely cleanup phase for a 2.0 release. These situations where Go doesn't respect its own modern conventions (eg. contexts and wrapped errors) is so jarring that it makes the language far too incomprehensible.

@networkimprov
Copy link

It's tagged for 1.16, the next release.

A lot of ppl wouldn't want Context to infect the rest of the stdlib. It's rather like "const poisoning" :-)

@dmitshur
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.16.
That time is now, so this is a friendly ping so the issue is looked at again.

@gopherbot
Copy link
Author

Change https://golang.org/cl/250357 mentions this issue: net: export ErrClosed

@ainar-g
Copy link
Contributor

ainar-g commented Aug 26, 2020

@ianlancetaylor A couple of years ago I proposed also exporting crypto/tls.errClosed, and you said that we should wait until this issue is resolved. Is now the right time for me to open a new proposal regarding that error?

@ianlancetaylor
Copy link
Contributor

@ainar-g Sure. Thanks. In particular, perhaps crypto/tls should replace errClosed with the new net.ErrClosed.

@awfm9
Copy link

awfm9 commented Aug 27, 2020

8.5 years later and we can finally remove this hacky code 🤣

@gopherbot
Copy link
Author

Change https://golang.org/cl/287592 mentions this issue: net/http: use net.ErrClosing instead of testing for string

r4g3baby added a commit to r4g3baby/mcserver that referenced this issue Mar 31, 2021
tbehling added a commit to hashicorp/go-plugin that referenced this issue Jan 27, 2022
Emit the message as DEBUG, so logs avoid false positive errors.

net.ErrClosed was added in Go 1.16, but this project's go.mod specifies
Go 1.13.  We alias Go's internal error type, to avoid the
worse alternative of comparing the string of the error message.

Go maintainers discuss this topic at length in
 golang/go#4373.
Occurences of "use of closed network connection"
are not an actual problem.  It's advisory only, akin to io.EOF.
@golang golang locked and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests