Skip to content

Address stacktrace.Propagate lib behavior of returning nil if err is nil #1255

@mickmis

Description

@mickmis

Is your feature request related to a problem? Please describe.
From stacktrace.Propagate documentation:

If cause is nil, Propagate returns nil. This allows elision of some "if err != nil" checks.

This choice IMO is very questionable because it violates the principle of least surprise.
E.g. this snippet of code (contributing cause of #1241):

func (gc *GarbageCollector) DeleteExpiredSubscriptions(ctx context.Context) error {
	expiredSubscriptions, err := gc.repos.ListExpiredSubscriptions(ctx, gc.writer)
	if err != nil {
		return stacktrace.Propagate(err,
			"Failed to list expired Subscriptions")
	}

	for _, sub := range expiredSubscriptions {
		subOut, err := gc.repos.DeleteSubscription(ctx, sub)
		if subOut != nil {
			return stacktrace.Propagate(err,
				"Deleted Subscription")
		}
		if err != nil {
			return stacktrace.Propagate(err,
				"Failed to delete Subscription")
		}
	}
	return nil
}

If subOut is nil, the returned error might very well be nil as well if err is nil. Benefits from avoiding some if err != nil checks is IMO by far not worth the possible confusion arising when reading such piece of code.

Describe the solution you'd like

  1. Change this behavior in https://github.com/interuss/stacktrace (unclear to me if other codebases are relying on this lib?)
  2. Remove https://github.com/interuss/stacktrace; import the lib as a package in this repo, and change this behavior

In both cases usages must be reviewed to assess the impact of this change. I would not be surprised if there were some buggy calls assuming that Propagate always return an error even if the error passed as a param is nil.

Describe alternatives you've considered
N/A

Additional context
#1241

Metadata

Metadata

Assignees

Labels

No labels
No labels

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions