-
Notifications
You must be signed in to change notification settings - Fork 293
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
multi: Use atomic types in unexported modules. #3053
Conversation
peer/peer.go
Outdated
@@ -1967,7 +1964,7 @@ func (p *Peer) start() error { | |||
// have no effect. | |||
func (p *Peer) AssociateConnection(conn net.Conn) { | |||
// Already connected? | |||
if !atomic.CompareAndSwapInt32(&p.connected, 0, 1) { | |||
if !p.connected.CompareAndSwap(false, true) { |
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.
I like how CompareAndSwap
was being used here to ensure actions are only performed once. I have copied this pattern to other places where it is appropriate (once in this file, twice in addrmanager.go
and once in rpcwebsocket.go
)
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.
Thanks for the PR.
Note that this change would have to bump the relevant go.mod
files to require Go 1.19 because the types were not introduced until Go 1.19.
I don't have any objections to the internal instances since they only require the bump to the main dcrd
module, but I'd prefer not to needlessly require newer Go versions for the other modules which hoist everyone else forward unless there is a really good reason and I don't think this qualifies.
So, in short, no objections for all of the one that touch the internal packages and code in the main module. However, I would prefer not to hoist everyone forward (at least yet) for the others.
Go <=1.18 no longer receives security patches. I'm fine leaving it behind. |
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.
I reviewed the changes to internal/*
and server.go
and they look good.
I didn't review the ones to addrmgr
, connmgr
, database
, peer
, or rpcclient
since they're in separate modules, and as previously mentioned, I don't want to hoist everyone forward for those.
That's a choice each consumer has to make with their own projects, imo. I don't think it's fair to force our own policies on everyone who uses our modules just to get updated versions of them in general. As someone who has been bitten many times by being on the receiving end of such policies, I'm particularly sensitive to it. For any cases where there is a legitimate security issue resolved that genuinely requires a new version, we'd obviously bump them. It might also be reasonable to follow the Go bootstrap toolchain as a minimum here since you won't even be able to build the relevant version of Go without that version. EDIT: Moved this discussion over to #3054. |
Exported modules are now updated in #3054. I've rolled the changes in this PR into one commit because they are all under a single |
go 1.19 introduced new atomic types which hide the underlying primitive types so that all accesses are forced to use the atomic APIs. This makes the code less prone to human error and a bit less verbose.
Of particular note is the introduction of the type
atomic.Bool
. Previously this repo made use of atomic integers to track properties with boolean values, but this PR updates those to useatomic.Bool
instead.