Fix go vet issues: example tests, returning a mutex copy #783
Conversation
Example tests are renamed to fit required naming conventions. This caused them to be relocated in generated docs. Also, a minor problem with writer.newBatchQueue is fixed, in order to avoid an unnecessary copy of a newly created object holding a mutex.
|
Looking at the batchQueue change again, I'm wondering if it's actually a significant change. I only made it mechanistically to make |
|
Hey @bhavanki, I just wanted to follow up, do you need any help to land this PR? |
writer.go
Outdated
| @@ -941,7 +941,7 @@ type partitionWriter struct { | |||
| func newPartitionWriter(w *Writer, key topicPartition) *partitionWriter { | |||
| writer := &partitionWriter{ | |||
| meta: key, | |||
| queue: newBatchQueue(10), | |||
| queue: *newBatchQueue(10), | |||
There was a problem hiding this comment.
Isn't this also a mutex copy?
There was a problem hiding this comment.
go vet doesn't complain about it, so perhaps not. If a pointer deference made a copy (in Go generally), then every dereference would produce a fresh copy and you'd never be able to work with the original. At least, that's my thinking, but I'm still new-ish to Go, so I'm happy to continue getting your insight!
There was a problem hiding this comment.
I don't believe the pointer dereference makes a copy but I think this initialization syntax turns into an assignment which would copy the dereferenced struct including the mutex.
There was a problem hiding this comment.
Oh, that's tricky! Thanks for the extra information!
There was a problem hiding this comment.
Found https://cs.opensource.google/go/x/tools/+/refs/tags/v0.1.7:go/analysis/passes/copylock/testdata/src/a/copylock_func.go;l=119
and
golang/go#16227
as well that talk about this exact case as well as copying zero valued locks being fine.
There was a problem hiding this comment.
@rhansen2 I reworked this change in a new commit back on Monday, so that this field is no longer initialized with a pointer dereference. But, I'm not sure now what kind of change you are in favor of, based on your last comment. Would you prefer that the code be reverted to copying mutexes as it originally did - since it's actually safe - even though vet complains about it?
There was a problem hiding this comment.
I'll defer to @achille-roussel on this.
There was a problem hiding this comment.
I agree that the ideal code shape would be to no have a pointer-to-mutex, as this is an uncommon pattern (sometimes presented as an anti-pattern), so despite being correct it may be cause of concerns for future readers.
An alternative approach could be to not have a constructor-style function, and instead use an initialization method:
func (bq *batchQueue) init(w *Writer, key topicPartition) {
// do all the initialization here, no mutex copies happen
...
}All that being said, a pragmatic move could be to accept the current change as it does fix the go vet warning and remains correct.
Up to you @bhavanki
There was a problem hiding this comment.
@achille-roussel I'll leave a comment with the concerns around introducing a pointer-to-mutex, and then merge what's here now. Someone can jump in later on and further revise the code to improve it more.
I've been preoccupied with some other more pressing issues, but I'm getting back to this one now! As long as folks continue to review and give feedback, it's all good. |
Instead of having newBatchQueue return a pointer to the entire batchQueue struct, instead make the mutex field within it a pointer, so that copying on return doesn't copy the mutex itself. The same treatment is needed for the cond field; the same no-copy rule applies to it.
Example tests are renamed to fit required naming conventions. This
caused them to be relocated in generated docs.
Also, a minor problem with writer.newBatchQueue is fixed, in order to
avoid an unnecessary copy of a newly created object holding a mutex.