-
Notifications
You must be signed in to change notification settings - Fork 813
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
Implements SDK callback for GameServer updates #316
Implements SDK callback for GameServer updates #316
Conversation
sdks/go/sdk.go
Outdated
s.watchCallbacks = append(s.watchCallbacks, f) | ||
var err error | ||
|
||
s.watchOnce.Do(func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@enocom I've love to get your take on this approach. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a sync.Once
, it might be easier to have a RegisterCallback
method to register a new callback. Then WatchGameServer
can run forever invoking the callbacks.
Presently, there's a data race between the call to append
above and reading the callbacks in the goroutine with range
below. If you split out the two concerns, you can lock around the slice of callbacks, taking out the lock when appending or reading from the slice, and then releasing it afterwords.
Be careful about locking around the callback, though, as there is a chance the callback might never finish. You might consider passing a content.Context
into the callback to provide a mechanism for cancelling the work and releasing the lock in failure cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading all this, and your comments I think I've got too fancy with this.
I think I'll take an approach that is similar to what I did with the C++ (although non-blocking). Each call to WatchGameServer
is it's own call to the gRPC service, with it's own singular function callback - that will make things far simpler to run and test, rather than managing sets of function callbacks and the potential issues therein.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -59,6 +59,18 @@ namespace agones { | |||
return stub->GetGameServer(context, request, response); | |||
} | |||
|
|||
grpc::Status SDK::WatchGameServer(const std::function<void(stable::agones::dev::sdk::GameServer)> callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kuqd @EricFortin My C++ is terrible - I hope I haven't done anything totally silly here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, may be @victor-prodan can confirm ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looks fine to me as well.
Build Succeeded 👏 Build Id: cd74a9a4-49c7-4773-bbee-2a8245e0ca5b The following development artifacts have been built, and will exist for the next 30 days:
|
This implements the gRPC system to send messages up to a unidirectional stream from the sdk-server to the SDK. This has been implemented in the Go, C++ and REST SDK. Closes googleforgames#277
774394c
to
6b7c090
Compare
Build Succeeded 👏 Build Id: d1fcc50e-e6db-4f74-8343-0d840928a255 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering why there is multiple streams, then I saw your discussion, LTGM
return errors.Wrap(err, "could not watch gameserver") | ||
} | ||
|
||
go func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markmandel Should this goroutine ever exit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - on EOF of the gRPC stream. Basically when the stream closes on the sdk sidecar server shutdown.
I could have written the if statement much cleaner looking at it now.
This implements the gRPC system to send messages up to a unidirectional
stream from the sdk-server to the SDK.
This has been implemented in the Go, C++ and REST SDK.
Closes #277