Application singleton is not really a singleton.

[twisted1919]

Description

It seems that the application singleton is not really a singleton, or something weird happens with my app.

If you take a look at:

you will see how some of my routes are registered twice.
I have a hunch this happens because in my app, i am also running a udp server, and inside it i defined a logger function like:

func logger() buffalo.Logger {
	return actions.App().Options.Logger
}

And i guess this somehow affects what's going on. It's just a hunch though.

However, if instead of:

    func App() *buffalo.App {
        if app == nil { ... }
    }

i'm using sync.Once:

    var appOnce sync.Once
    func App() *buffalo.App {
        appOnce.Do(func() {  ...  })
    }

Then my issue goes away, no more duplicate routes.

Steps to Reproduce the Problem

See above.

Expected Behavior

I expect to see unique routes.

Actual Behavior

I see duplicate routes

[sio4]

Indeed, this issue (action.App() is called multiple times while app is still nil) could very rarely happen since we don't use any locking method such as sync.Once on our scaffold. Also, App itself is not a singleton from the perspective of the core library, but it could be or have to be from the perspective of the user application.

However, it will not happen frequently (actually not happen in most case) since our scaffold also has init() function calling actions.App() in grifts/init.go before main() is executed. (this example [1] too) In this case, the variable actions.app will not be nil when actions.App() is called from other places including the main() of the app, but yes, it is not guaranteed when the app is somewhat complex.

But I am not sure how it happened... if the user-defined logger() which calls App() is called... from where and when...

I think we can improve the scaffolded app.go with Sync but still, this is a user's domain.

[1] http://gobuffalo.io/documentation/guides/goth/#example-usage

[fasmat]

Indeed, this issue (action.App() is called multiple times while app is still nil) could very rarely happen since we don't use any locking method such as sync.Once on our scaffold. Also, App itself is not a singleton from the perspective of the core library, but it could be or have to be from the perspective of the user application.

However, it will not happen frequently (actually not happen in most case) since our scaffold also has init() function calling actions.App() in grifts/init.go before main() is executed. (this example [1] too) In this case, the variable actions.app will not be nil when actions.App() is called from other places including the main() of the app, but yes, it is not guaranteed when the app is somewhat complex.

But I am not sure how it happened... if the user-defined logger() which calls App() is called... from where and when...

I think we can improve the scaffolded app.go with Sync but still, this is a user's domain.

[1] http://gobuffalo.io/documentation/guides/goth/#example-usage

We should still make sure that the code generated with buffalo new doesn't contain any possible race conditions that should be fixed by the users.

The generated actions.App() function is not safe to be called from multiple goroutines, but it should be imho. It's also a straight forward fix: wrap the initialization of app with a sync.Once.

var app *buffalo.App

func App() *buffalo.App {
	if app == nil {
		app = buffalo.New(buffalo.Options{
			Env:         ENV,
			SessionName: "_buffalo_session",
		})

		// **SNIP**
	}

	return app

should be

var app *buffalo.App
var appInit sync.Once

func App() *buffalo.App {
	appInit.Do(func() {
		app = buffalo.New(buffalo.Options{
			Env:         ENV,
			SessionName: "_buffalo_session",
		})

		// **SNIP**
	})

	return app
}

I just haven't had time yet to update the necessary templates in the gobuffalo cli and write a fixer for it.

[sio4]

We should still make sure that the code generated with buffalo new doesn't contain any possible race conditions that should be fixed by the users.

The generated actions.App() function is not safe to be called from multiple goroutines, but it should be imho. It's also a straight forward fix: wrap the initialization of app with a sync.Once.

Yeah, I fully agree. Having a possible bug in the generated code for the user is not a good thing. I think adding that sync could be fine.

Also, at the same time, I am curious about what made this very edge case happen. As described above, grifts/init.go could be the first caller of the function, and this stage, when init() invoking, I don't think goroutines starting... 🤔
(Issue is too old to ask the situation... )