gobreaker icon indicating copy to clipboard operation
gobreaker copied to clipboard

[Go 1.21] Replace 'any' / 'interface{}' implementation with Generics

Open zalgonoise opened this issue 2 years ago • 5 comments

This PR proposes swapping the any, or interface{} type with a generic type, for the CircuitBreaker's Execute method.

This method takes in one parameter, a function with no input parameters and two return elements: an empty interface interface{} and an error.

With Go 1.18, we're able to replace some of this type of code to ensure a faster execution with less type switching and casting -- by defining that, in this case, your CircuitBreaker's response type is X (be it a byte slice, or a custom type), the runtime will take care of returning the correct type. This means that there is no longer a cast to a type by the end of the Execute method in the example, which ideally would also have a check if buf, ok := body.([]byte); ok { return buf, nil }.

Also, since we're jumping from Go 1.12 to Go 1.18 in this proposal, we might as well just focus on the latest Go version available. This would ensure the best performance and security benefits from the latest Go runtime and standard library -- but it should be open for discussion in this PR, as it would imply a major version bump nevertheless.

Looking forward for your review. :)

zalgonoise avatar Oct 18 '23 17:10 zalgonoise

This is lovely and an absolute quickwin. Maybe it should be release under version v2 though to allow backwards compatibility? Would love to have this feature even if it slightly reduces flexibility to operate on different response types with one cb instance.

niksteff avatar Feb 13 '24 10:02 niksteff

Awesome @niksteff, I will get started with a v2 package for this PR ASAP, then. I agree it's the best approach to promote backwards compatibility :D

zalgonoise avatar Feb 13 '24 11:02 zalgonoise

@YoshiyukiMineo is there a possibility this PR could be merged in a not so distant future? Would really dig the change. Or if you people at Sony have no interest we could fork this I think :) Just need a confirmation of some kind.

And for the people just strolling by and looking for a solution to kind of implement generics maybe this func can be an inspiration:

func Break[T any](cb *gobreaker.CircuitBreaker, f func() (T, error)) (T, error) {
	res, err := cb.Execute(func() (interface{}, error) {
		res, err := f()
		if err != nil {
			return nil, fmt.Errorf("error executing circuit breaker func: %w", err)
		}

		return res, nil
	})
	if err != nil {
		return *new(T), err
	}

	return res.(T), nil
}

niksteff avatar Feb 14 '24 07:02 niksteff

@zalgonoise @niksteff

Thank you for your prposals. I will merge this PR after v2 package is introduced for backward compatibility.

YoshiyukiMineo avatar Feb 15 '24 02:02 YoshiyukiMineo

@YoshiyukiMineo

Thank you for your time and consideration! I've updated my fork to separate the generics implementation into a new module within this repo (as github.com/sony/gobreaker/v2).

Please take your time to review the PR again, and feel free to share any ideas on what you'd like to see different. :)

zalgonoise avatar Feb 15 '24 15:02 zalgonoise

Is there a reason prefer the v2 directory strategy over using a v2 tags?

pior avatar Apr 11 '24 17:04 pior

Is there a reason prefer the v2 directory strategy over using a v2 tags?

Hi @pior

The proposal allows new (breaking) changes to be added while still keeping the original API maintainable and usable.

This follows Go's recommended versioning model: https://go.dev/doc/modules/version-numbers

If we were to replace the logic in-place of the old one, then a patch or a fix would be exponentially harder deploy for v1 since it would be lost in the commit history. And since v2 could drift into more changes, then you'd have to re-apply all changes to a branch off the last v1.

All of this is avoidable if v1 is still available. If suddenly there is a need to upgrade dependencies to both versions due to, for example, a CVE, this can be done for both v1 and v2 at ease.

zalgonoise avatar Apr 11 '24 20:04 zalgonoise

Hello guys, do you have an estimated date to approve this PR and move forward?

drbornot avatar Apr 26 '24 15:04 drbornot

I have merged this PR into v2 branch. I will merge v2 branch into master branch after modifying it slightly.

YoshiyukiMineo avatar Apr 30 '24 15:04 YoshiyukiMineo