Repository navigation
lint: Enable channelcheck linter - #5056
Conversation
djb15
left a comment
There was a problem hiding this comment.
Spent a good amount of time reviewing this, I do think the logical changes are reasonable but I'd like another reviewer to feel the same. Generally there is a minority of changes here from blocking to non-blocking on the error channels, and I believe they're all shared by numerous senders, so going to non-blocking makes sense.
6f4a90d to
7e54391
Compare
djb15
left a comment
There was a problem hiding this comment.
Approving. Just fixed a small merge conflict in spy.go
bemic
left a comment
There was a problem hiding this comment.
Left two comments. The likelihood of the potential issue is very low. If we agree and accept it, I can discard and approve
d5aa04c to
ca40486
Compare
evan-gray
left a comment
There was a problem hiding this comment.
did my best to review all these, though I'm not intimately familiar with all the code paths.
NOTE This PR was ported from @mdulin2's PR at 4875, which became a bit tangled in merge conflicts.
This is part two of integrating the channel linter into the repository. Part one added the custom linter flow, and the channel linter; this enables the channel linter.
In general, three types of changes were made:
WriteToChannelWithoutBlockinghelper. Done onerrCand metric channel writes mostly.ctx.Done()to theselectfor smoother shutdown.nolint:channelcheckwith an explanation on why the current setup is correct.It should be noted that
msgCis made as an exception to thechannelchecklinter because it should always be a blocking write. With the addition of thenolintlinter, we can't addnolint:channelcheckwith a comment there. So, I left the originalNote on channel capacitycomment as is.