Always defer *sql.Rows.Close and consult with Err (#844)

* Always defer *sql.Rows.Close and consult with Err

database/sql.Rows.Next() makes sure to call Close only after exhausting
result rows which would NOT happen when returning early from a bad Scan.
Close being idempotent makes it a great candidate to get always deferred
regardless of what happens later on the result set.

This change also makes sure call Err() after exhausting Next() and
propagate non-nil results from it as the documentation advises.

Closes #764

Signed-off-by: Kiril Vladimiroff <kiril@vladimiroff.org>

* Override named result parameters in last returns

Signed-off-by: Kiril Vladimiroff <kiril@vladimiroff.org>

* Do the same over new changes that got merged

Signed-off-by: Kiril Vladimiroff <kiril@vladimiroff.org>

Co-authored-by: Neil Alexander <neilalexander@users.noreply.github.com>

6 files changed, 15 insertions, 11 deletions

diff --git a/syncapi/storage/postgres/account_data_table.go b/syncapi/storage/postgres/account_data_table.go
index 94e6ac41..3d75ad99 100644
--- a/syncapi/storage/postgres/account_data_table.go
+++ b/syncapi/storage/postgres/account_data_table.go
@@ -118,6 +118,7 @@ func (s *accountDataStatements) selectAccountDataInRange(
 	if err != nil {
 		return
 	}
+	defer rows.Close() // nolint: errcheck
 
 	for rows.Next() {
 		var dataType string
@@ -133,8 +134,7 @@ func (s *accountDataStatements) selectAccountDataInRange(
 			data[roomID] = []string{dataType}
 		}
 	}
-
-	return
+	return data, rows.Err()
 }
 
 func (s *accountDataStatements) selectMaxAccountDataID(
diff --git a/syncapi/storage/postgres/backward_extremities_table.go b/syncapi/storage/postgres/backward_extremities_table.go
index 1489f7f9..d63c546e 100644
--- a/syncapi/storage/postgres/backward_extremities_table.go
+++ b/syncapi/storage/postgres/backward_extremities_table.go
@@ -91,6 +91,7 @@ func (s *backwardExtremitiesStatements) selectBackwardExtremitiesForRoom(
 	if err != nil {
 		return
 	}
+	defer rows.Close() // nolint: errcheck
 
 	for rows.Next() {
 		var eID string
@@ -101,7 +102,7 @@ func (s *backwardExtremitiesStatements) selectBackwardExtremitiesForRoom(
 		eventIDs = append(eventIDs, eID)
 	}
 
-	return
+	return eventIDs, rows.Err()
 }
 
 func (s *backwardExtremitiesStatements) isBackwardExtremity(
diff --git a/syncapi/storage/postgres/current_room_state_table.go b/syncapi/storage/postgres/current_room_state_table.go
index 816cbb44..117bd775 100644
--- a/syncapi/storage/postgres/current_room_state_table.go
+++ b/syncapi/storage/postgres/current_room_state_table.go
@@ -154,7 +154,7 @@ func (s *currentRoomStateStatements) selectJoinedUsers(
 		users = append(users, userID)
 		result[roomID] = users
 	}
-	return result, nil
+	return result, rows.Err()
 }
 
 // SelectRoomIDsWithMembership returns the list of room IDs which have the given user in the given membership state.
@@ -179,7 +179,7 @@ func (s *currentRoomStateStatements) selectRoomIDsWithMembership(
 		}
 		result = append(result, roomID)
 	}
-	return result, nil
+	return result, rows.Err()
 }
 
 // CurrentState returns all the current state events for the given room.
@@ -267,7 +267,7 @@ func rowsToEvents(rows *sql.Rows) ([]gomatrixserverlib.Event, error) {
 		}
 		result = append(result, ev)
 	}
-	return result, nil
+	return result, rows.Err()
 }
 
 func (s *currentRoomStateStatements) selectStateEvent(
diff --git a/syncapi/storage/postgres/invites_table.go b/syncapi/storage/postgres/invites_table.go
index ca4bbeb5..2cb8fb19 100644
--- a/syncapi/storage/postgres/invites_table.go
+++ b/syncapi/storage/postgres/invites_table.go
@@ -133,7 +133,7 @@ func (s *inviteEventsStatements) selectInviteEventsInRange(
 
 		result[roomID] = event
 	}
-	return result, nil
+	return result, rows.Err()
 }
 
 func (s *inviteEventsStatements) selectMaxInviteID(
diff --git a/syncapi/storage/postgres/output_room_events_table.go b/syncapi/storage/postgres/output_room_events_table.go
index 6d213a57..610ff23e 100644
--- a/syncapi/storage/postgres/output_room_events_table.go
+++ b/syncapi/storage/postgres/output_room_events_table.go
@@ -170,6 +170,7 @@ func (s *outputRoomEventsStatements) selectStateInRange(
 	if err != nil {
 		return nil, nil, err
 	}
+	defer rows.Close() // nolint: errcheck
 	// Fetch all the state change events for all rooms between the two positions then loop each event and:
 	//  - Keep a cache of the event by ID (99% of state change events are for the event itself)
 	//  - For each room ID, build up an array of event IDs which represents cumulative adds/removes
@@ -226,7 +227,7 @@ func (s *outputRoomEventsStatements) selectStateInRange(
 		}
 	}
 
-	return stateNeeded, eventIDToEvent, nil
+	return stateNeeded, eventIDToEvent, rows.Err()
 }
 
 // MaxID returns the ID of the last inserted event in this table. 'txn' is optional. If it is not supplied,
@@ -392,5 +393,5 @@ func rowsToStreamEvents(rows *sql.Rows) ([]types.StreamEvent, error) {
 			ExcludeFromSync: excludeFromSync,
 		})
 	}
-	return result, nil
+	return result, rows.Err()
 }
diff --git a/syncapi/storage/postgres/output_room_events_topology_table.go b/syncapi/storage/postgres/output_room_events_topology_table.go
index 793d1e23..78a381da 100644
--- a/syncapi/storage/postgres/output_room_events_topology_table.go
+++ b/syncapi/storage/postgres/output_room_events_topology_table.go
@@ -134,6 +134,7 @@ func (s *outputRoomEventsTopologyStatements) selectEventIDsInRange(
 	} else if err != nil {
 		return
 	}
+	defer rows.Close() // nolint: errcheck
 
 	// Return the IDs.
 	var eventID string
@@ -144,7 +145,7 @@ func (s *outputRoomEventsTopologyStatements) selectEventIDsInRange(
 		eventIDs = append(eventIDs, eventID)
 	}
 
-	return
+	return eventIDs, rows.Err()
 }
 
 // selectPositionInTopology returns the position of a given event in the
@@ -176,6 +177,7 @@ func (s *outputRoomEventsTopologyStatements) selectEventIDsFromPosition(
 	} else if err != nil {
 		return
 	}
+	defer rows.Close() // nolint: errcheck
 	// Return the IDs.
 	var eventID string
 	for rows.Next() {
@@ -184,5 +186,5 @@ func (s *outputRoomEventsTopologyStatements) selectEventIDsFromPosition(
 		}
 		eventIDs = append(eventIDs, eventID)
 	}
-	return
+	return eventIDs, rows.Err()
 }