IND-2885 Linting #641

KaushikiAnand · 2025-04-08T06:27:08Z

I have added linting to go-tests.yml.
The linting was done to ensure that the codebase is consistent and maintainable and helps in improving the code quality and reducing errors.

abhijeetviswa · 2025-04-08T12:17:18Z

net_transport.go

@@ -313,7 +314,9 @@ func (n *NetworkTransport) CloseStreams() {
 	// entry.
 	for k, e := range n.connPool {
 		for _, conn := range e {
-			conn.Release()
+			if err := conn.Release(); err != nil {
+				log.Print(err)


Suggestion: A more descriptive error message here would be helpful when tracing the error.

abhijeetviswa · 2025-04-08T12:17:23Z

net_transport.go

@@ -441,7 +442,9 @@ func (n *NetworkTransport) returnConn(conn *netConn) {
 	if !n.IsShutdown() && len(conns) < n.maxPool {
 		n.connPool[key] = append(conns, conn)
 	} else {
-		conn.Release()
+		if err := conn.Release(); err != nil {
+			log.Print(err)


Same as above.

abhijeetviswa · 2025-04-08T12:18:48Z

net_transport.go

+	var errors error
+	defer func() {
+		if err := conn.Release(); err != nil && errors == nil {
+			errors = err


Question: Does setting it to errors server any purpose? I don't see it being used anywhere below.

abhijeetviswa · 2025-04-08T12:23:19Z

net_transport_test.go

@@ -635,7 +635,9 @@ func TestNetworkTransport_InstallSnapshot(t *testing.T) {

 				// Try to read the bytes
 				buf := make([]byte, 10)
-				rpc.Reader.Read(buf)
+				if _, err := rpc.Reader.Read(buf); err != nil {
+					t.Errorf("unable to read bytes: %v", err)


Suggestion: Can we wrap the error instead of formatting it into the string i.e %v -> %w?

There are a bunch of similar instances in raft.go as well.

abhijeetviswa · 2025-04-08T12:25:33Z

raft_test.go

@@ -1859,7 +1859,7 @@ func TestRaft_Barrier(t *testing.T) {
 	// Ensure all the logs are the same
 	c.EnsureSame(t)
 	if len(getMockFSM(c.fsms[0]).logs) != 100 {
-		t.Fatalf(fmt.Sprintf("Bad log length: %d", len(getMockFSM(c.fsms[0]).logs)))
+		t.Fatalf("%s", fmt.Sprintf("Bad log length: %d", len(getMockFSM(c.fsms[0]).logs)))


Question: Why not use t.Fatal(...) instead?

tgross

We need to be very careful about introducing behavior changes and swallowing important errors in production code for the sake of silencing lints. If we can't justify each change aside from lints I'm a -1 on this PR.

tgross · 2025-04-08T13:00:25Z

net_transport.go

+		if err := conn.Release(); err != nil {
+			return err
+		}


What's a circumstance where we're going to get an error here and want that to be the return value instead of the original error? (Similar for all these changes.)

tgross · 2025-04-08T13:02:39Z

net_transport.go

+		if err := n.conn.conn.SetWriteDeadline(time.Now().Add(timeout)); err != nil {
+			return nil, err
+		}


How does this change behavior in the caller? We're changing the error handling in many places in this PR. We're doing this for the sake of linting without much in the way of consideration for how it changes the expected behavior. "It silences linters" isn't a good enough reason to make this change without that consideration.

(This comment applies to a lot of these.)

tgross · 2025-04-08T13:04:06Z

raft.go

+		if err := sink.Cancel(); err != nil {
+			return fmt.Errorf("failed to cancel snapshot: %v", err)
+		}
 		return fmt.Errorf("failed to write snapshot: %v", err)


Similar to the case I described in net_transport.go, we're now swallowing the original error which is much more important for the caller to understand what's going on. This also applies anywhere else in this PR where we're doing this (there's a half dozen I can see).

mukeshjc · 2025-04-09T12:07:09Z

@tgross
Thank you for the review. We have discussed this internally and plan to take below approach for moving forward:

For all instances of teardown functions (eg: conn.Resolve(), sink.Cancel() etc.) that are flagged by the linter, we will exclude them to suppress the findings since they aren't needed for teardown type of functions.
Many errcheck specific lint failures were for unit test files, so we will add config to ignore test files from that linter.

Thoughts?

tgross · 2025-04-09T13:09:12Z

For all instances of teardown functions (eg: conn.Resolve(), sink.Cancel() etc.) that are flagged by the linter, we will exclude them to suppress the findings since they aren't needed for teardown type of functions.

That's a reasonable approach. Another option would be to figure out which teardown functions we do need to care about and get the added error context wrapped in the original error we return. But that's certainly more work and needs more attention.

Many errcheck specific lint failures were for unit test files, so we will add config to ignore test files from that linter.

Why? It seems totally safe to add the extra checks for unit tests, right?

mukeshjc · 2025-04-10T05:43:00Z

Why? It seems totally safe to add the extra checks for unit tests, right?

The idea is fixing these linting checks in unit test files doesn't improve the actual library. I'm happy to include them too, no strong opinions here. We will keep them.

tgross · 2025-04-10T13:27:47Z

The idea is fixing these linting checks in unit test files doesn't improve the actual library. I'm happy to include them too, no strong opinions here. We will keep them.

This is a little all-or-nothing. Either the lint is useful, in which case you should apply it where it's safe to apply or it's not useful at all. I think it is useful, so applying it to test code in particular could be handy.

Your change in e1a5e6a leaves a number of cases in the production code, however.

mukeshjc · 2025-04-11T05:15:30Z

Your change in e1a5e6a leaves a number of cases in the production code, however.

@tgross

We plan to re-visit the need for error wrapping of teardown functions after syncing with the group in #tech-raft slack channel. Based on the feedback we will keep/remove the suppressed findings.

KaushikiAnand added 4 commits April 8, 2025 11:19

lint check added

e4182e5

lint cmd added

a4a7cca

linter error solved

b81c0be

IND-2885 lint check added to workflow

1172234

KaushikiAnand requested review from a team as code owners April 8, 2025 06:27

KaushikiAnand requested a review from jmurret April 8, 2025 06:27

abhijeetviswa requested changes Apr 8, 2025

View reviewed changes

tgross requested changes Apr 8, 2025

View reviewed changes

teardown function errchecks been supppresed as suggested

e1a5e6a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IND-2885 Linting #641

IND-2885 Linting #641

KaushikiAnand commented Apr 8, 2025

abhijeetviswa Apr 8, 2025

abhijeetviswa Apr 8, 2025

abhijeetviswa Apr 8, 2025

abhijeetviswa Apr 8, 2025

abhijeetviswa Apr 8, 2025

abhijeetviswa Apr 8, 2025

tgross left a comment

tgross Apr 8, 2025

tgross Apr 8, 2025

tgross Apr 8, 2025

mukeshjc commented Apr 9, 2025

tgross commented Apr 9, 2025

mukeshjc commented Apr 10, 2025

tgross commented Apr 10, 2025

mukeshjc commented Apr 11, 2025 •

edited

Loading

IND-2885 Linting #641

Are you sure you want to change the base?

IND-2885 Linting #641

Conversation

KaushikiAnand commented Apr 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukeshjc commented Apr 9, 2025

tgross commented Apr 9, 2025

mukeshjc commented Apr 10, 2025

tgross commented Apr 10, 2025

mukeshjc commented Apr 11, 2025 • edited Loading

mukeshjc commented Apr 11, 2025 •

edited

Loading