Implements SDK callback for GameServer updates #316
Conversation
markmandel
added
kind/feature
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.
@enocom I've love to get your take on this approach. WDYT?
There was a problem hiding this comment.
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.
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.
| @@ -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.
@Kuqd @EricFortin My C++ is terrible - I hope I haven't done anything totally silly here?
There was a problem hiding this comment.
looks good to me, may be @victor-prodan can confirm ?
There was a problem hiding this comment.
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
markmandel
force-pushed
the
feature/gs-callback
branch
from
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:
|
| return errors.Wrap(err, "could not watch gameserver") | ||
| } | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
@markmandel Should this goroutine ever exit?
There was a problem hiding this comment.
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