Skip to content

chore: Happy path on the left, early return #1461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 4, 2024
Merged

chore: Happy path on the left, early return #1461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f64535f
util: Improve readability with early return
breml Dec 4, 2024
b9f7d5b
incusd/db: Improve readability with eraly return
breml Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 27 additions & 25 deletions internal/server/db/query/retry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,35 +23,37 @@ const maxRetries = 250
//
// This should by typically used to wrap transactions.
func Retry(ctx context.Context, f func(ctx context.Context) error) error {
// TODO: the retry loop should be configurable.
var err error
for i := 0; i < maxRetries; i++ {
err = f(ctx)
if err != nil {
if errors.Is(err, context.Canceled) {
break
}

// No point in re-trying or logging a no-row or not found error.
if errors.Is(err, sql.ErrNoRows) || api.StatusErrorCheck(err, http.StatusNotFound) {
break
}

// Process actual errors.
if IsRetriableError(err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we could actually flatten this further if we changed the beginning of the logic to:

err = f(ctx)
if err == nil {
    break
}

ERROR HANDLING LOGIC

That would let us get rid of that weird break at the end.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd go with something like this these days:

// Retry wraps a function that interacts with the database, and retries it in
// case a transient error is hit.
//
// This should by typically used to wrap transactions.
func Retry(ctx context.Context, f func(ctx context.Context) error) error {
	var err error

	for i := 0; i < maxRetries; i++ {
		err = f(ctx)
		if err == nil {
			// The function succeeded, we're done here.
			break
		}

		if errors.Is(err, context.Canceled) {
			// The function was canceled, don't retry.
			break
		}

		if errors.Is(err, sql.ErrNoRows) || api.StatusErrorCheck(err, http.StatusNotFound) {
			// No point in re-trying or logging a no-row or not found error.
			break
		}

		// Process actual errors.
		if !IsRetriableError(err) {
			logger.Debug("Database error", logger.Ctx{"err": err})
			break
		}

		if i == maxRetries {
			logger.Warn("Database error, giving up", logger.Ctx{"attempt": i, "err": err})
			break
		}

		logger.Debug("Database error, retrying", logger.Ctx{"attempt": i, "err": err})
		time.Sleep(jitter.Deviation(nil, 0.8)(100 * time.Millisecond))

		continue
	}

	return err
}

(I removed the TODO given that we've not actually had a good reason to make this configurable in the past decade ;))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the PR as you proposed. In general, I am not a huge fan of if == nil, since I prefer to have the happy path on the left, but in this case I can see the benefit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, even the final continue became obsolete, since at the end of the loop body this is what happens anyway.

if i == maxRetries {
logger.Warn("Database error, giving up", logger.Ctx{"attempt": i, "err": err})
break
}

logger.Debug("Database error, retrying", logger.Ctx{"attempt": i, "err": err})
time.Sleep(jitter.Deviation(nil, 0.8)(100 * time.Millisecond))
continue
} else {
logger.Debug("Database error", logger.Ctx{"err": err})
}
if err == nil {
// The function succeeded, we're done here.
break
}
break

if errors.Is(err, context.Canceled) {
// The function was canceled, don't retry.
break
}

// No point in re-trying or logging a no-row or not found error.
if errors.Is(err, sql.ErrNoRows) || api.StatusErrorCheck(err, http.StatusNotFound) {
break
}

// Process actual errors.
if !IsRetriableError(err) {
logger.Debug("Database error", logger.Ctx{"err": err})
break
}

if i == maxRetries {
logger.Warn("Database error, giving up", logger.Ctx{"attempt": i, "err": err})
break
}

logger.Debug("Database error, retrying", logger.Ctx{"attempt": i, "err": err})
time.Sleep(jitter.Deviation(nil, 0.8)(100 * time.Millisecond))
}

return err
Expand Down
16 changes: 9 additions & 7 deletions internal/util/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,17 @@ func CanonicalNetworkAddress(address string, defaultPort int) string {
if ip != nil {
// If the input address is a bare IP address, then convert it to a proper listen address
// using the canonical IP with default port and wrap IPv6 addresses in square brackets.
address = net.JoinHostPort(ip.String(), fmt.Sprintf("%d", defaultPort))
} else {
// Otherwise assume this is either a host name or a partial address (e.g `[::]`) without
// a port number, so append the default port.
address = fmt.Sprintf("%s:%d", address, defaultPort)
return net.JoinHostPort(ip.String(), fmt.Sprintf("%d", defaultPort))
}
} else if port == "" && address[len(address)-1] == ':' {

// Otherwise assume this is either a host name or a partial address (e.g `[::]`) without
// a port number, so append the default port.
return fmt.Sprintf("%s:%d", address, defaultPort)
}

if port == "" && address[len(address)-1] == ':' {
// An address that ends with a trailing colon will be parsed as having an empty port.
address = net.JoinHostPort(host, fmt.Sprintf("%d", defaultPort))
return net.JoinHostPort(host, fmt.Sprintf("%d", defaultPort))
}

return address
Expand Down
Loading