-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Fix warnings, possible linter failures and typos #223
Changes from all commits
2ac1c2f
504d6c6
c0578ed
bc45e5e
6d67795
d575728
26e05cc
1b79b20
ab2e0f4
1021230
364833f
9ba9e08
ad9feb3
cbf5888
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import ( | |
"testing" | ||
|
||
"github.com/caddyserver/caddy/v2/caddytest" | ||
|
||
_ "github.com/mholt/caddy-l4" | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,7 @@ import ( | |
) | ||
|
||
func init() { | ||
caddy.RegisterModule(App{}) | ||
caddy.RegisterModule(&App{}) | ||
} | ||
|
||
// App is a Caddy app that operates closest to layer 4 of the OSI model. | ||
|
@@ -40,7 +40,7 @@ type App struct { | |
} | ||
|
||
// CaddyModule returns the Caddy module information. | ||
func (App) CaddyModule() caddy.ModuleInfo { | ||
func (*App) CaddyModule() caddy.ModuleInfo { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, this doesn't need to be a pointer. Maybe it doesn't matter. I don't think I'm used to using pointers for the Module registration stuff. 🤷♂️ |
||
return caddy.ModuleInfo{ | ||
ID: "layer4", | ||
New: func() caddy.Module { return new(App) }, | ||
|
@@ -76,11 +76,11 @@ func (a *App) Start() error { | |
case net.Listener: | ||
a.listeners = append(a.listeners, ln) | ||
lnAddr = caddy.JoinNetworkAddress(ln.Addr().Network(), ln.Addr().String(), "") | ||
go s.serve(ln) | ||
go func() { _ = s.serve(ln) }() | ||
case net.PacketConn: | ||
a.packetConns = append(a.packetConns, ln) | ||
lnAddr = caddy.JoinNetworkAddress(ln.LocalAddr().Network(), ln.LocalAddr().String(), "") | ||
go s.servePacket(ln) | ||
go func() { _ = s.servePacket(ln) }() | ||
} | ||
s.logger.Debug("listening", zap.String("address", lnAddr)) | ||
} | ||
|
@@ -90,7 +90,7 @@ func (a *App) Start() error { | |
} | ||
|
||
// Stop stops the servers and closes all listeners. | ||
func (a App) Stop() error { | ||
func (a *App) Stop() error { | ||
for _, pc := range a.packetConns { | ||
err := pc.Close() | ||
if err != nil { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ import ( | |
) | ||
|
||
func init() { | ||
caddy.RegisterModule(ListenerWrapper{}) | ||
caddy.RegisterModule(&ListenerWrapper{}) | ||
} | ||
|
||
// ListenerWrapper is a Caddy module that wraps App as a listener wrapper, it doesn't support udp. | ||
|
@@ -33,7 +33,7 @@ type ListenerWrapper struct { | |
} | ||
|
||
// CaddyModule returns the Caddy module information. | ||
func (ListenerWrapper) CaddyModule() caddy.ModuleInfo { | ||
func (*ListenerWrapper) CaddyModule() caddy.ModuleInfo { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't finished my review yet but does this actually solve a problem? I guess I don't care either way in the cases where this isn't even used (there's not even a variable named for it) but I guess I just am curious. |
||
return caddy.ModuleInfo{ | ||
ID: "caddy.listeners.layer4", | ||
New: func() caddy.Module { return new(ListenerWrapper) }, | ||
|
@@ -126,7 +126,8 @@ type listener struct { | |
func (l *listener) loop() { | ||
for { | ||
conn, err := l.Listener.Accept() | ||
if nerr, ok := err.(net.Error); ok && nerr.Temporary() { | ||
var nerr net.Error | ||
if errors.As(err, &nerr) && nerr.Temporary() { | ||
l.logger.Error("temporary error accepting connection", zap.Error(err)) | ||
continue | ||
} | ||
|
@@ -145,7 +146,7 @@ func (l *listener) loop() { | |
close(l.connChan) | ||
}() | ||
for conn := range l.connChan { | ||
conn.Close() | ||
_ = conn.Close() | ||
} | ||
} | ||
|
||
|
@@ -156,8 +157,8 @@ func (l *listener) handle(conn net.Conn) { | |
var err error | ||
defer func() { | ||
l.wg.Done() | ||
if err != errHijacked { | ||
conn.Close() | ||
if !errors.Is(err, errHijacked) { | ||
_ = conn.Close() | ||
} | ||
}() | ||
|
||
|
@@ -171,7 +172,7 @@ func (l *listener) handle(conn net.Conn) { | |
start := time.Now() | ||
err = l.compiledRoute.Handle(cx) | ||
duration := time.Since(start) | ||
if err != nil && err != errHijacked { | ||
if err != nil && !errors.Is(err, errHijacked) { | ||
l.logger.Error("handling connection", zap.Error(err)) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Registering a module doesn't require an allocation, it just needs an example value.
(Same with other module(s) below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the value/pointer receiver warnings, my approach is as follows. We do need a pointer receiver in Provision functions, so that fields could be updated in a struct. We don’t have functions where a value receiver is required, so that no field would be updated unintentionally. Golang allows structs to have both value and pointer receivers, but it’s recommended to avoid mixing them for a single struct - that’s what the warning basically says. Thus, my suggestion is to use pointer receivers everywhere unless value receivers are strictly required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the module registration is never the same instance being used for provisioning. Those are separate. I'm not sure what linter you're using, but it's definitely overly aggressive on that. I don't think I agree with that rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I’m talking about is written in the golang official docs. E.g. see https://go.dev/tour/methods/8, paragraph 5. It’s a general rule to have either value or pointer receivers for all struct functions, but not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module is now registered here with & just because CaddyModule function has a pointer receiver instead of a value receiver it used to have. My tests show this way of module registration works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting... and yeah, I mean, it'll work... it's just unusual in terms of Caddy convention.
(I think our JSON docs generator handles pointer values here correctly when it does its static analysis.)
Typically what I do is determine whether a pointer or value receiver is the correct/conservative thing to use, and choose that. If the value isn't used, or if a copy is permissible (no values from the sync package, for example, in the struct), I prefer a value receiver, I think this avoids an allocation? I'm not sure. But it also makes nil pointer derefs impossible which is nice.
Anyway... I dunno. Still feels weird to use a pointer when it's not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we have basically 2 options here:
If there is more support for the first option, I will adjust this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after asking teh Twitter what they do, the results are really mixed. Many use only pointer receivers (consistency, avoids bugs). A good number only use pointers if that is most appropriate (self-documenting code, mutable structs, etc).
I am fine with the change as-is I guess :) Thanks!