Enable Channel Check Linter#4875
Open
mdulin2 wants to merge 18 commits into
Open
Conversation
mdulin2
marked this pull request as ready for review
July 16, 2026 15:18
mdulin2
requested review from
SEJeff,
bemic,
djb15,
evan-gray,
johnsaigle,
kcsongor,
panoel and
pleasew8t
as code owners
July 16, 2026 15:18
Contributor
|
@mdulin2 looks like this needs a rebase |
…hannel handling changes
mdulin2
force-pushed
the
channel-check-enable
branch
from
July 20, 2026 19:41
163170d to
5e25970
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
johnsaigle
reviewed
Jul 21, 2026
johnsaigle
left a comment
johnsaigle
left a comment
Contributor
There was a problem hiding this comment.
The changes to the channel handling seem sensible. It's also nice to see more detail getting added to the nolint comments 👍🏻
I'm happy to approve after the Near comments are updated.
| if msg != nil { | ||
| msg.IsReobservation = true | ||
| w.msgChan <- msg | ||
| w.msgChan <- msg // Note on channel capacity: We never want to drop messages. |
Contributor
There was a problem hiding this comment.
Should the changes to this file be nolint instead of the "Note" style?
| // If we go back too far, we just report the error and terminate early. | ||
| if recursionDepth > maxFallBehindBlocks { | ||
| e.eventChan <- EVENT_NEAR_WATCHER_TOO_FAR_BEHIND // Note on channel capacity: Only pauses this watcher | ||
| common.WriteToChannelWithoutBlocking(e.eventChan, EVENT_NEAR_WATCHER_TOO_FAR_BEHIND, "near_event_chan") // Non-blocking: eventChan is buffered (cap 10) and metrics-only, so dropping an event is preferable to stalling the watcher. |
Contributor
There was a problem hiding this comment.
suggestion: Avoid putting constants directly in documentation in case they change later
Suggested change
| common.WriteToChannelWithoutBlocking(e.eventChan, EVENT_NEAR_WATCHER_TOO_FAR_BEHIND, "near_event_chan") // Non-blocking: eventChan is buffered (cap 10) and metrics-only, so dropping an event is preferable to stalling the watcher. | |
| common.WriteToChannelWithoutBlocking(e.eventChan, EVENT_NEAR_WATCHER_TOO_FAR_BEHIND, "near_event_chan") // Non-blocking: eventChan is buffered and metrics-only, so dropping an event is preferable to stalling the watcher. |
Same with the cap 10 comment in the other Near file
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.