Fix simple-game-server to use context substitute for the infinite loop #3050
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Build Succeeded 👏 Build Id: e260f53e-830d-4103-9036-5c5a13d46019 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
examples/simple-game-server/main.go
Outdated
| @@ -36,7 +36,8 @@ import ( | |||
|
|
|||
| // main starts a UDP or TCP server | |||
| func main() { | |||
| go doSignal() | |||
| sigCtx, sigCancel := context.WithCancel(context.Background()) | |||
There was a problem hiding this comment.
Rather than keeping doSignal, could we simplify and remove that function, and instead do:
ctx := signals.NewSigKillContext()
And make a minor change to the section below (line 125) of:
<-ctx.Done()
os.Exit(0)
and we don' t need to worry about wrapping contexts or anything like that.
WDYT?
There was a problem hiding this comment.
@markmandel
I agree with that.
I'll fix it.
But var ctx is already used at the below lines to manage health context.
ctx, cancel := context.WithCancel(context.Background())
go doHealth(s, ctx)
So should we use sigCtx to manage signal context like the one below?
func main() {
sigCtx := signals.NewSigKillContext()
~~~//other tasks
<-sigCtx.Done()
os.Exit(0)
}
//doSignal is erased.
Or should we rename ctx which is used to go doHealth(s, ctx) like healthCtx then keep using ctx on main like that?
func main() {
ctx := signals.NewSigKillContext()
~~~
healthCtx, cancel := context.WithCancel(context.Background())
go doHealth(s, healthCtx)
~~~
<-ctx.Done()
os.Exit(0)
}
//doSignal is erased.
There was a problem hiding this comment.
Or should we rename ctx which is used to go doHealth(s, ctx) like healthCtx then keep using ctx on main like that?
Either way, I don't mind. I'll leave the decision up to you 🤗
|
Also looking at the CLA failure - looks like you signed with your github account - this one doesn't have your CLA signed. But it looks like your personal email has signed the CLA, so we'll need you to rebase down commits with your personal email please. |
I'm sorry about that.
I just resigned my CLA on my GitHub account which was signed on my commit. |
|
Build Succeeded 👏 Build Id: f99684ec-a166-45d6-957a-62f2368fd847 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Failed 😱 Build Id: 2d0369fd-2428-4199-8963-0f5bf1527d82 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
There was a problem hiding this comment.
Thanks this is great.
You will need to resolve the CLA issue. The best bet is to Git rebase your PR down to a single commit, and make sure the author is your gmail account, and force push it over the top of this branch.
You can see which accounts are passing and which aren't here: https://github.com/googleforgames/agones/pull/3050/checks?check_run_id=12403813562
These may be useful:
|
Also you can ignore the Agones bot failure, it's just being flaky. |
|
Build Succeeded 👏 Build Id: 0cd09d02-bced-4097-958a-a7dfbaa0e472 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Succeeded 👏 Build Id: 60fef5ef-1693-4431-8bbf-262ac37d6c4f The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Thanks for providing the information. It seems to pass the CLA check. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markmandel, oniku-2929 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Lovely! |
|
New changes are detected. LGTM label has been removed. |
|
Build Failed 😱 Build Id: 04a4be1b-becf-44ad-b3d9-4a9a6e346f8b To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
zmerlynn
changed the title
zmerlynn
changed the title
|
Build Succeeded 👏 Build Id: 31254eb8-a3f9-40a0-93df-e32e65601889 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
…op at main (googleforgames#2208). (googleforgames#3050) Co-authored-by: oniku2929 <o29n1ku2ku@gmail.com> Co-authored-by: Mark Mandel <markmandel@google.com>
…op at main (#2208).
What type of PR is this?
What this PR does / Why we need it:
for reducing simple game server cpu usage.
#2208
Which issue(s) this PR fixes:
Closes
Special notes for your reviewer: