cmd/commandfuncs: use net.ErrClosed instead of doing substring error checks [requires go 1.16]

[jimen0]

Go 1.16 will introduce a new exported net.ErrClosed (see this issue for context) error allowing Caddy to properly check for this error instead of using the substring matching hack.

Current code:

caddy/cmd/commandfuncs.go

Lines 115 to 123 in 385adf5

	go func() {
	for {
	conn, err := ln.Accept()
	if err != nil {
	if !strings.Contains(err.Error(), "use of closed network connection") {
	log.Println(err)
	}
	break
	}

Proposed patch once Caddy starts using go 1.16:

diff --git a/cmd/commandfuncs.go b/cmd/commandfuncs.go
index 772fe012..36c686c5 100644
--- a/cmd/commandfuncs.go
+++ b/cmd/commandfuncs.go
@@ -19,6 +19,7 @@ import (
 	"context"
 	"crypto/rand"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -116,7 +117,7 @@ func cmdStart(fl Flags) (int, error) {
 		for {
 			conn, err := ln.Accept()
 			if err != nil {
-				if !strings.Contains(err.Error(), "use of closed network connection") {
+				if !errors.Is(err, net.ErrClosed) {
 					log.Println(err)
 				}
 				break

Please, let me know what do you think about it and, if you would prefer me to open the PR now or if I should wait for 1.16 to be released. Also, please feel free to close this issue if you don't think we need this code change.

Best,
Miguel

[francislavoie]

This is interesting, but I'm not sure we can realistically make this change soon if it will require us to bump up the minimum Go version. We want to keep a reasonably low minimum version to maintain compatibility with the Go version used by the different platforms we support, e.g. Fedora/Centos.

Probably best to wait a couple more versions to let the ecosystem to catch up before we bump minimum.

[jimen0]

Hi @mholt @francislavoie 1.17beta1 is out. Should I prepare the PR and submit it once 1.17 is out or would you prefer me waiting until 1.18 is released? 😄

[francislavoie]

Thanks for the reminder @jimen0!

We're keeping an eye on https://src.fedoraproject.org/rpms/golang and https://access.redhat.com/support/policy/updates/rhel8-app-streams-life-cycle (bottom of the page) to see what their supported Go versions are. Since 1.16 isn't supported across the board yet, I think we'll wait a bit longer.

We could bump the mimimum regardless, but it would take the COPR maintainers a bit of extra work to wire up the Caddy package to use a later version than their default.

/cc @carlwgeorge, who informed me of the above details 👍

[carlwgeorge]

Right now the problematic distros are Fedora 33, RHEL 7, and RHEL 8. RHEL 8 will get golang 1.16 approximately in November. Fedora 33 will be EOL about the same time so it won't be a concern either at that point. I haven't heard anything about golang getting upgraded past 1.15 for RHEL 7 (which comes from the community EPEL repo), but if that's the last one that is an issue we can do the necessary override in our COPR build root. If waiting until November is unacceptable, we'll set up the overrides for all of them.

[jimen0]

From my side this can wait as much as needed. It's just code cleanup and not something urgent. I'll set up a reminder for mid-November to come back to this PR. Thank you a lot for your time @carlwgeorge. This is what makes opensource so awesome!

[francislavoie]

FYI @jimen0 we're readying to bump to 1.16 very soon #4283

[jimen0]

Thanks for the ping @francislavoie! My calendar was marked for November. I submitted the PR.

No rush to review it at all. Let me know if anything else is needed from my side and happy 1.17 migration!

[mholt]

Yeah sorry it's early; I thought November would be the release date, but I guess not.