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

Race condition in startup #254

Closed
KnicKnic opened this issue Jan 15, 2020 · 4 comments · Fixed by #261
Closed

Race condition in startup #254

KnicKnic opened this issue Jan 15, 2020 · 4 comments · Fixed by #261
Labels
Milestone

Comments

@KnicKnic
Copy link
Contributor

I found this while trying to embed gobetween and not run it standalone so it may not hit as easily in normal case, but the race is still there.

gobetween/main.go

Lines 83 to 89 in 7d8a736

go api.Start((*cfg).Api)
/* setup metrics */
go metrics.Start((*cfg).Metrics)
// Start manager
go manager.Initialize(*cfg)

These calls all initialize defaults, however it is possible for the them not to complete before API calls get in. This then ends up in dereference of null.

I hit it in this case.

*server.MaxConnections = *defaults.MaxConnections

Ideally what main should do is run all of these synchronously to init them, and only then launch agents.

api.Initialize()
metrics.Initialize()
manager.Initialize()
go api.Start()
go metrics.Start()
go manager.Start()
@yyyar
Copy link
Owner

yyyar commented Jan 16, 2020

Could you please show the exact error message you got and steps to reproduce?

@KnicKnic
Copy link
Contributor Author

KnicKnic commented Jan 16, 2020

.\test.exe  
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0xa6c23e]

goroutine 1 [running]:
github.com/yyyar/gobetween/manager.prepareConfig(0xc129cb, 0x3, 0xc00018a048, 0x0, 0x0, 0x0, 0xc1961f, 0xd, 0xc12abe, 0x3, ...)
        C:/Users/nmaliwa/go/src/github.com/yyyar/gobetween/src/manager/manager.go:572 +0xbce
github.com/yyyar/gobetween/manager.Create(0xc129cb, 0x3, 0x0, 0x0, 0x0, 0x0, 0xc1961f, 0xd, 0xc12abe, 0x3, ...)
        C:/Users/nmaliwa/go/src/github.com/yyyar/gobetween/src/manager/manager.go:193 +0x218
main.main()
        C:/Users/nmaliwa/go/src/github.com/yyyar/gobetween/src/test/testapp.go:26 +0x1e4

so I effectively coppied your main.go into launch\launch.go
I then used the launch package to embed into a test app and add servers using the manager package.

see my repro commit
KnicKnic@d454094

@yyyar
Copy link
Owner

yyyar commented Jan 16, 2020

Thanks, I See. I suppose, for standalone gobetween, just reordering like this may be enough to fix a race condition:

 // Init metrics
metrics.Start((*cfg).Metrics)

// Init manager
manager.Initialize(*cfg)

// Start API
go api.Start((*cfg).Api)

@yyyar yyyar added the bug label Jan 16, 2020
@yyyar
Copy link
Owner

yyyar commented Jan 23, 2020

fixed by #261

@yyyar yyyar closed this as completed Jan 23, 2020
@yyyar yyyar added this to the 0.8.0 milestone Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants