Skip to content
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.

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

Do not use closures to prevent some data races #2398

Open
leonklingele opened this issue Apr 2, 2023 · 2 comments
Open

Do not use closures to prevent some data races #2398

leonklingele opened this issue Apr 2, 2023 · 2 comments

Comments

@leonklingele
Copy link
Member

leonklingele commented Apr 2, 2023

In order to permanently avoid the problem of #2396, we should start and switch to never return anonymous functions (closures) but return non-anonymous functions instead so no data is shared — never. For middlewares this might look as follows:

// New creates a new middleware handler
func New(config ...Config) fiber.Handler {
	// Setup, etc.

	// Return new handler
	return newHandler(list, shared, data, here, config, etc)
}

func newHandler(list, shared, data, here, config, etc) func(c *fiber.Ctx) {
    // Important: Never do anything in here before returning the closure.
    return func(c *fiber.Ctx) {
        // Code of handler as before, but without access to data of setup logic
    }
}

Originally posted by @leonklingele in #2396 (comment)

@ReneWerner87
Copy link
Member

we should definitely check the benchmarks afterwards to make sure that the allocations have remained identical.

Since the framework is trimmed for speed, this has priority over possible error prevention in this case.
but hope this should not conflict at all and of course works without changing the memory accesses

@leonklingele
Copy link
Member Author

we should definitely check the benchmarks afterwards to make sure that the allocations have remained identical.

Go should inline those calls. We can verify using https://github.com/nikolaydubina/go-recipes#-show-compiler-optimization-decisions-on-heap-and-inlining

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants