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

Reduce the risk of panic #197

Closed
1 of 2 tasks
seeflood opened this issue Sep 1, 2021 · 6 comments · Fixed by #208
Closed
1 of 2 tasks

Reduce the risk of panic #197

seeflood opened this issue Sep 1, 2021 · 6 comments · Fixed by #208
Assignees
Labels
help wanted Extra attention is needed kind/enhancement New feature or request priority/P2

Comments

@seeflood
Copy link
Member

seeflood commented Sep 1, 2021

What would you like to be added:
Layotto(open source version) will be imported and deployed in the production environment.
Thanks to golang's weird design,a panic in a third-party library will bring down the whole process, which means a panic bug can bring down the whole cluster.
Since that, we must reduce the risk of panic before deploying Layotto in the production environment.

Tasks included:

  • replace all code which starts a new goroutine(e.g.go func(){xxx} ) with recover phrase( e.g. utils.GoWithRecover(func() { xxx } )
  • Let users import components on demand.Currently we import all components in the main.go and it has the risk of dependency conflicts.To reduce the risk,we can let users decide which components should be imported and which should not.But it takes time to do some research and careful design.
    image

chinese:

  • 把所有创建新协程的地方(e.g.go func(){xxx})加上recover(直接用工具类即可, utils.GoWithRecover(func() { xxx } )
  • 让用户按需import需要的组件。目前是main.go里默认import加载所有组件,万一有依赖冲突之类的事情很危险。可以改造成用啥组件才import啥,但是方案需要调研下

注:新手任务只需要做第一个就行(给所有创建新协程的地方加上recover),不用管第二个,第二个我来调研一下

Why is this needed:
Make Layotto more robust

@seeflood seeflood added kind/enhancement New feature or request help wanted Extra attention is needed priority/P2 labels Sep 1, 2021
@arcosx
Copy link
Member

arcosx commented Sep 3, 2021

I am happy to do this work, can you assign this issue to me?

@seeflood
Copy link
Member Author

seeflood commented Sep 3, 2021

I am happy to do this work, can you assign this issue to me?

Cool !
Welcome to the Layotto community

@x-shadow-man
Copy link
Contributor

I'm very excited to do this work, pls assign this issue to me! thx a lot

@seeflood
Copy link
Member Author

seeflood commented Sep 3, 2021

I'm very excited to do this work, pls assign this issue to me! thx a lot

Ah,sorry that I just assigned this issue to @arcosx
@x-shadow-man Would you like to choose another task in the task list #108
Any unmarked task in the list is unassigned and needs help
Thanks for your passion !

@arcosx
Copy link
Member

arcosx commented Sep 12, 2021

Sorry for my late submission🤣, I submitted a PR #208 regarding the first solution point: replace all code which starts a new goroutine.......

One question, should my changes involve the directories demo,sdk, and _test.go ?
The reason for this is that I noticed that demo,sdk does not contain mosn.io/pkg/utils/, would it be redundant to introduce it, and would _test.go not actually affect the deployment of the production environment.

@seeflood
Copy link
Member Author

seeflood commented Sep 13, 2021

@arcosx Thanks for your contribution!
I think sdk package also need recover,while demo and _test.go don't. If panic happens during testing ,let it crash and we can easily find out the bugs.
There is no need to introduce mosn.io/pkg/utils/ into sdk .We can write a new GoWithRecover in sdk or just copy the existing mosn.io/pkg/utils/goroutine.go into the sdk package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed kind/enhancement New feature or request priority/P2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants